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

Removal of support for bool? #647

Closed
johnlees opened this issue Nov 11, 2022 · 5 comments · Fixed by #654
Closed

Removal of support for bool? #647

johnlees opened this issue Nov 11, 2022 · 5 comments · Fixed by #654

Comments

@johnlees
Copy link

johnlees commented Nov 11, 2022

Describe the bug
When trying to store a bool with:

bool on = true;
HighFive::Group group = h5_file.createGroup(name, true);
HighFive::Attribute reads_a = group.createAttribute<bool>("is_on", HighFive::DataSpace::From(on);

Two compiler errors occur due to a static_assert added in v2.5.0

To Reproduce
Either see the following build:

Steps to reproduce the behavior:

  1. Go to BLAS linking; CI fix; better message for deprecated function bacpop/pp-sketchlib#84
  2. Install environment.yml with conda
  3. Run python setup.py build

Expected behavior
Build without failure, no static_assert issue

Stacktrace
The following compiler errors occur:

2022-11-11T12:29:34.2177653Z       /usr/share/miniconda/envs/pp_env/include/highfive/bits/../bits/H5Converter_misc.hpp:83:19: error: static assertion failed: Booleans are not supported yet.
2022-11-11T12:29:34.2178241Z          83 |     static_assert(!std::is_same<type, bool>::value, "Booleans are not supported yet.");
2022-11-11T12:29:34.2178658Z             |                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2022-11-11T12:29:34.2179569Z       /usr/share/miniconda/envs/pp_env/include/highfive/bits/../bits/H5Converter_misc.hpp: In instantiation of ‘const hdf5_type* HighFive::details::Writer<T>::get_pointer() [with T = bool; HighFive::details::Writer<T>::hdf5_type = bool]’:
2022-11-11T12:29:34.2180635Z       /usr/share/miniconda/envs/pp_env/include/highfive/bits/H5Attribute_misc.hpp:116:5:   required from ‘void HighFive::Attribute::write(const T&) [with T = bool]’
2022-11-11T12:29:34.2181661Z       /usr/share/miniconda/envs/pp_env/include/highfive/bits/../bits/H5Converter_misc.hpp:647:29: error: void value not ignored as it ought to be
2022-11-11T12:29:34.2182092Z         647 |             return vec.data();
2022-11-11T12:29:34.2182347Z             |                             ^

2022-11-11T12:29:34.2185810Z       /usr/share/miniconda/envs/pp_env/include/highfive/bits/H5ReadWrite_misc.hpp:109:1: error: ‘HighFive::details::BufferInfo<T>::BufferInfo(const HighFive::DataType&, F, HighFive::details::BufferInfo<T>::Operation) [with F = HighFive::Attribute::read(T&) const [with T = bool]::<lambda()>; T = bool]’, declared using local type ‘HighFive::Attribute::read(T&) const [with T = bool]::<lambda()>’, is used but never defined [-fpermissive]
2022-11-11T12:29:34.2186910Z         109 | BufferInfo<T>::BufferInfo(const DataType& dtype, F getName, Operation _op)
2022-11-11T12:29:34.2187265Z             | ^~~~~~~~~~~~~

Desktop (please complete the following information):

  • OS: ubuntu 20.10
  • Version: HighFive v2.6.2

Additional context
This appears to be due to this static_assert: https://github.com/BlueBrain/HighFive/blob/master/include/highfive/bits/H5Converter_misc.hpp#L83

Which seems to have be introduced in:
#586

So I think this has effectively removed support for storing a single bool, and I don't see this deprecation in any of the release notes?
I see that there is discussion about vector<bool> not yet being supported, but it seems even a single bool has been removed, which is awkward for backwards compatibility.

I also see that #591 reported something similar.

@1uc
Copy link
Collaborator

1uc commented Nov 21, 2022

Thank you for reporting the issue. I'm able to reproduce it locally, we'll look into it. If you urgently require any of the newly introduced functionality, you might be able to work around the issue by storing/loading a std::uint8_t instead, something along the lines of:

HighFive::File file(FILE_NAME);
HighFive::Group group = file.createGroup(GROUP_NAME, true);
group.createAttribute(ATTR_NAME, std::uint8_t{true});

auto attr = bool(group.getAttribute(ATTR_NAME).read<std::uint8_t>());

@johnlees
Copy link
Author

Thanks, I've indeed rewritten our code using uint8_t now (I see them in the DB as H5T_STD_U8LE) – just need to check it works ok with the older DBs too.
Although, do you plan on adding bool support back in? In which case I might revert my changes and pin an older highfive version until this is added again

@1uc
Copy link
Collaborator

1uc commented Nov 21, 2022

I would be nice to simply store/load bools. One thing that makes this a bit more finicky is that HDF5 doesn't seem to include a native type for bool. Therefore, we'll probably try to do whatever h5py does. It seems it builds an 'enum' from a signed 8-bit integer as follows:

$ h5dump bool.h5
HDF5 "bool.h5" {
GROUP "/" {
   DATASET "false" {
      DATATYPE  H5T_ENUM {
         H5T_STD_I8LE;
         "FALSE"            0;
         "TRUE"             1;
      }
      DATASPACE  SCALAR
      DATA {
      (0): FALSE
      }
   }
   DATASET "true" {
      DATATYPE  H5T_ENUM {
         H5T_STD_I8LE;
         "FALSE"            0;
         "TRUE"             1;
      }
      DATASPACE  SCALAR
      DATA {
      (0): TRUE
      }
   }
}
}

This is also what you see in the PR you link, for reintroducing support for bool.

@johnlees
Copy link
Author

Makes a lot of sense.
However I want to stay backwards compatible with HDF5 files generated with previous versions of my code, so I'll go ahead and use the uint8_t approach

@alkino
Copy link
Member

alkino commented Jan 23, 2023

@johnlees
You have to know that the support of booleans inside HighFive and h5py, is done by an enum that is a uint8_t (0: false, 1: true), so you will be able to use bool type with the old versions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants