-
Notifications
You must be signed in to change notification settings - Fork 51
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Runtime error reading bp files #1221
Comments
What is happening here You have written a dataset with a vector containing one empty string
In reading, the openPMD-api does not distinguish between single-element vectors and scalar values in ADIOS2 attributes, so it sees this as a single empty string. Empty strings however are disallowed because they are not supported by our HDF5 backend and we want to keep things compatible with all backends. Possible workaround We could disable this kind of check while reading and restrict the check to writing. For now, #1223 implements this workaround and should make things work for you again. With this fix, I can read your dataset without issue. Better fix In the output above, |
@franzpoeschel See I am also surprised that HDF5 wouldn't support empty strings. I was able to create an HDF5 file with an empty string attribute just fine:
|
So the functionality definitely exists, but it seems like it is currently not exposed in the C++ bindings (your link is from the C bindings). I've already contacted Norbert about this, maybe he sees something that I don't.
To be fair, the HDF5 backend was written before I started working on openPMD, so Axel will know more about the details here probably |
Thanks for investigating this and the first work-around in #1223 👍 HDF5I think the reason we did not support this is because a couple of the involved C calls in both read and write need extra careful handling if used with empty strings, e.g., for vlen strings: #1084 For instance, there are calls like Just to close the loop for writing as well: Do you have a good use case why we should in write not error out for empty strings? This feels a bit like if we add support for it, then we might loose a lot of time debugging it for old releases of various backends & platforms, etc. triage it with upstream, add extra code paths for work-arounds, and I don't immediately see a use case yet. I am ok-ish to add it in read. |
I checked, and neither Python nor Julia have any issues reading or writing empty string attributes in HDF5. It should be possible to continue to support writing empty strings, even for HDF5 files. The error reported above was for an ADIOS2 file and was thus unrelated to HDF5. My case for empty string is: It is always good to support empty containers, and this is often a natural way of expressing certain conditions. Programming languages support empty strings. The file mentioned above contains an empty string because that was a natural choice in our writer. It is, of course, always possible to avoid empty strings, but this can be nontrivial. The file above contains an array of strings with one array element per patch. Avoiding empty strings in an array either requires remapping array indices (that would add much complexity), or adding a sentinel value to each string, or to empty strings. The reader then needs to look for and handle these sentinel values. Paying respect to Fortran, one could replace empty strings with a string that contains only a single space |
I see, thanks for the detailed example. We usually solves this in openPMD keywords by explicitly having If we add sufficent testing for it in a PR, then I think it is worth adding support for it. We should check in the tests that ADIOS2 and HDF5 backends handle this well. |
Let's continue this in a new issue for tracking? |
Hi! I have been using openpmd-api for a while and I did not have any issues reading .bp files with openPMD-api in Python. Recently I am facing an issue with reading bp files. When I execute:
I get the following error:
RuntimeError: [setAttribute] Value for string attribute 'patchSuffixes' must not be empty!
I am using openpmd-api 0.14.4 and I have attached the bp file.
planewave_08.it00000000.zip
The text was updated successfully, but these errors were encountered: