Skip to content
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

bool: h5py compatible #50

Closed
ax3l opened this issue Feb 10, 2018 · 9 comments
Closed

bool: h5py compatible #50

ax3l opened this issue Feb 10, 2018 · 9 comments

Comments

@ax3l
Copy link
Member

ax3l commented Feb 10, 2018

Representing bool types in HDF5 is not pre-defined in HDF5 and can e.g. be implemented via enums.

Those are a little tricky and even case sensitive but can be implemented in a widely accepted way, that e.g. h5py understands. See the implementation in libSplash for reference:

https://github.com/ComputationalRadiationPhysics/libSplash/blob/v1.7.0/src/include/splash/basetypes/ColTypeBool.hpp#L35-L89

@C0nsultant
Copy link
Member

I took the guts of the problem straight from splash and integrated them into the HDF5 backend.

Writing a bool works such that the value is h5py compliant. Reading apparently does not. I'll close the issue once I've verified that reading this non-native bool works (here).

@ax3l
Copy link
Member Author

ax3l commented Feb 11, 2018

In the link above, we check for bools as follows in splash during reads:

        // in: hid_t datatype_id

        bool found = false;
        H5T_class_t h5_class = H5Tget_class(datatype_id);
        if(h5_class == H5T_ENUM)
        {
            if(H5Tget_nmembers(datatype_id) == 2)
            {
                char* m0 = H5Tget_member_name(datatype_id,0);
                char* m1 = H5Tget_member_name(datatype_id,1);
                if(strcmp("TRUE" , m0) == 0 && strcmp("FALSE", m1) == 0)
                    found = true;
                free(m1);
                free(m0);
            }
        }

If I remember correctly, it's important to check a datatype for ints before "bools" before other enums:
https://github.com/ComputationalRadiationPhysics/libSplash/blob/v1.7.0/src/generateCollectionType.cpp#L67-L127

@C0nsultant
Copy link
Member

@ax3l Is there any intent to support arrays of bools (as opposed to bitfields in unsigned integers) as an Attribute datatype?

@ax3l
Copy link
Member Author

ax3l commented Feb 12, 2018

Tbh, I don't get the question.

Yes, single and arrays of bools are useful attributes in general. As long as stuff is h5py compatible it's fine and afaik h5py does not support bitfields as bools?

@C0nsultant
Copy link
Member

Let me try again:

The standard never mentions bool explicitly. So the functionality in this issue is for generality and merely convenience for the user.

What I was trying to get at: Do you see any need for explicit vectors/arrays of bools? By means of of uint 8-, 16-, 32- and 64- sized arrays are possible. With this enum-feature implemented, shall we support explicit arrays of bool with arbitrary length?

@ax3l
Copy link
Member Author

ax3l commented Feb 12, 2018

You mean as individual date (H5T_class_t)?

I am currently only aware of applications where we use e.g. a 1D particle record of bool and single scalar attributes of bool.

Anyway, openPMD is agnostic in what users choose as actual data types.

@C0nsultant
Copy link
Member

The merged PR only handles single scalar attributes. No arrays of attributes or records.

@ax3l
Copy link
Member Author

ax3l commented Feb 13, 2018

I leave this open until we have a CI step in which we read a generated output with h5py.
I have to add more to the CI anyway (GCC 6, pep8/pyflakes, etc.)

@ax3l
Copy link
Member Author

ax3l commented Jan 15, 2020

h5py bool compatibility is part of CI now.
Complex is similarly planned via #639

@ax3l ax3l closed this as completed Jan 15, 2020
First Stable Release automation moved this from To do to Done Jan 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

No branches or pull requests

2 participants