[Einstein Toolkit] #1050: [PATCH]CarpetIOHDF5: make WriteLargeAttribute write a string, not an array of ints
#1050: [PATCH]CarpetIOHDF5: make WriteLargeAttribute write a string, not an array of ints -------------------------------+-------------------------------------------- Reporter: anton@… | Owner: eschnett Type: defect | Status: new Priority: minor | Milestone: Component: Carpet | Version: Keywords: | -------------------------------+-------------------------------------------- The data that's supplied to it is to be interpreted as a string.
Before the attached patch the data is written as an array of int8, e.g.: h5dump -d '/Parameters and Global Attributes/Datasets' alpha.h5 HDF5 "alpha.h5" { DATASET "/Parameters and Global Attributes/Datasets" { DATATYPE H5T_STD_I8LE DATASPACE SIMPLE { ( 16 ) / ( 16 ) } DATA { (0): 77, 76, 95, 66, 83, 83, 78, 58, 58, 97, 108, 112, 104, 97, 10, 0 } } }
After the patch: h5dump -d '/Parameters and Global Attributes/Datasets' alpha.h5 HDF5 "alpha.h5" { DATASET "/Parameters and Global Attributes/Datasets" { DATATYPE H5T_STRING { STRSIZE 16; STRPAD H5T_STR_NULLTERM; CSET H5T_CSET_ASCII; CTYPE H5T_C_S1; } DATASPACE SIMPLE { ( 1 ) / ( 1 ) } DATA { (0): "ML_BSSN::alpha " } } }
#1050: [PATCH]CarpetIOHDF5: make WriteLargeAttribute write a string, not an array of ints --------------------------------+------------------------------------------- Reporter: anton@… | Owner: eschnett Type: defect | Status: new Priority: minor | Milestone: Component: Carpet | Version: Resolution: | Keywords: --------------------------------+-------------------------------------------
Comment (by rhaas):
Does HDF5 still read in the string attribute as a CHAR array later on (eg. in the checkpoint input routine, I do not know if this is used anywhere else but to checkpoint/recover the parameter string from IOUtil)? Looks harmless otherwise :-).
#1050: [PATCH]CarpetIOHDF5: make WriteLargeAttribute write a string, not an array of ints --------------------------------+------------------------------------------- Reporter: anton@… | Owner: eschnett Type: defect | Status: new Priority: minor | Milestone: Component: Carpet | Version: Resolution: | Keywords: --------------------------------+-------------------------------------------
Comment (by eschnett):
Carpet currently writes an array of characters (not an array of integers), thus the difference between this and a string is small.
The patch does not close the new datatype, leading to a memory leak.
The readers of these attributes, used mostly during recovery, also need to be updated to read strings and not expect arrays of characters.
#1050: [PATCH]CarpetIOHDF5: make WriteLargeAttribute write a string, not an array of ints --------------------------------+------------------------------------------- Reporter: anton@… | Owner: eschnett Type: defect | Status: new Priority: minor | Milestone: Component: Carpet | Version: Resolution: | Keywords: --------------------------------+-------------------------------------------
Comment (by rhaas):
silly question: do we know if trac actually automatically sends emails to the email of the reporter? Just in case anton is not subscribed to the ET mailing list.
#1050: [PATCH]CarpetIOHDF5: make WriteLargeAttribute write a string, not an array of ints --------------------------------+------------------------------------------- Reporter: anton@… | Owner: eschnett Type: defect | Status: new Priority: minor | Milestone: Component: Carpet | Version: Resolution: | Keywords: --------------------------------+-------------------------------------------
Comment (by anton@…):
Replying to [comment:2 eschnett]:
Carpet currently writes an array of characters (not an array of
integers), thus the difference between this and a string is small.
Right. To clarify, I'm using this in my python script for processing the data, since it seemed the most "proper" way to get grid attributes. Or is it a better idea to use something else?
Of course I could do ''.join(map(chr, *)) on it, but writing it properly in the first place seems cleaner.
The patch does not close the new datatype, leading to a memory leak.
The readers of these attributes, used mostly during recovery, also need
to be updated to read strings and not expect arrays of characters.
Sorry about this, I'm not very familiar with the code yet. Should be fixed in the new patch.
#1050: [PATCH]CarpetIOHDF5: make WriteLargeAttribute write a string, not an array of ints --------------------------------+------------------------------------------- Reporter: anton@… | Owner: eschnett Type: defect | Status: new Priority: minor | Milestone: Component: Carpet | Version: Resolution: | Keywords: --------------------------------+-------------------------------------------
Comment (by eschnett):
No need to be sorry! That's what a review is for, and I thank you for improving our code.
I am not overly familiar with strings in HDF5. I notice that the documentation of H5D_get_storage_size says that it is generally not recommended to use this function. H5T_get_size (the converse of the function used to create the string type) may be more appropriate. Could you update your patch?
Did you run the test cases after applying your patch? I assume that (a) this patch indeed makes it simpler for Python to read the attributes, and (b) that in particular the recovery test cases succeed.
To answer an earlier question: Yes, these attributes are how the grid structure is stored and retrieved. We opted for a human-readable (i.e. ASCII) format to simplify debugging.
#1050: [PATCH]CarpetIOHDF5: make WriteLargeAttribute write a string, not an array of ints --------------------------------+------------------------------------------- Reporter: anton@… | Owner: eschnett Type: defect | Status: new Priority: minor | Milestone: Component: Carpet | Version: Resolution: | Keywords: --------------------------------+-------------------------------------------
Comment (by anton@…):
Replying to [comment:5 eschnett]:
No need to be sorry! That's what a review is for, and I thank you for
improving our code.
I am not overly familiar with strings in HDF5. I notice that the
documentation of H5D_get_storage_size says that it is generally not recommended to use this function. H5T_get_size (the converse of the function used to create the string type) may be more appropriate. Could you update your patch?
Ok, done. It makes the patch slightly longer, but I guess a cleaner solution is better.
Did you run the test cases after applying your patch? I assume that (a)
this patch indeed makes it simpler for Python to read the attributes, and (b) that in particular the recovery test cases succeed.
Yes, specifically the CarpetWaveToyRecover_test_*proc passes both with and without the changes to the checkpoint file.
I'm a bit confused by the nobuffers test claiming No files created in test directory
Success: 0 files identical
but that seems unrelated to the patch.
#1050: [PATCH]CarpetIOHDF5: make WriteLargeAttribute write a string, not an array of ints --------------------------------+------------------------------------------- Reporter: anton@… | Owner: eschnett Type: defect | Status: review Priority: minor | Milestone: Component: Carpet | Version: Resolution: | Keywords: --------------------------------+------------------------------------------- Changes (by eschnett):
* status: new => review
Comment:
Okay, the patch looks good. I will apply it.
#1050: [PATCH]CarpetIOHDF5: make WriteLargeAttribute write a string, not an array of ints --------------------------------+------------------------------------------- Reporter: anton@… | Owner: eschnett Type: defect | Status: reviewed_ok Priority: minor | Milestone: Component: Carpet | Version: Resolution: | Keywords: --------------------------------+------------------------------------------- Changes (by eschnett):
* status: review => reviewed_ok
#1050: [PATCH]CarpetIOHDF5: make WriteLargeAttribute write a string, not an array of ints --------------------------------+------------------------------------------- Reporter: anton@… | Owner: eschnett Type: defect | Status: closed Priority: minor | Milestone: Component: Carpet | Version: Resolution: fixed | Keywords: --------------------------------+------------------------------------------- Changes (by eschnett):
* status: reviewed_ok => closed * resolution: => fixed
Comment:
Applied.
trac@lists.einsteintoolkit.org
-
Einstein Toolkit