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

Documentation and clean up #22

Merged
merged 10 commits into from
Apr 24, 2024
Merged

Conversation

Thoemi09
Copy link
Contributor

This PR is very similar to the one from itertools and mpi. The following things have been done:

  • formatted .clang-tidy file
  • added c++ documentation
  • split object.hpp into multiple headers
  • removed all sphinx and added doxygen related files
  • updated the github action workflow
  • fixed a bug in the h5::file constructor (-e option was not working)

I think we should consider either restricting or improving the h5::array_interface. Right now, it is kept quite general but only works for specific cases, e.g. contiguous data/views, hyperslabs with blocks of size (1, 1, ..., 1) and so on. It probably would be good to only support our specific use cases and otherwise throw an exception.

@Thoemi09 Thoemi09 requested a review from Wentzell April 18, 2024 23:04
@Thoemi09 Thoemi09 force-pushed the DEV_DOCUMENTATION branch 4 times, most recently from e92465e to 0792c9d Compare April 19, 2024 14:12
@Thoemi09 Thoemi09 force-pushed the DEV_DOCUMENTATION branch from 0792c9d to a93a9e7 Compare April 23, 2024 15:55
Copy link
Member

@Wentzell Wentzell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @Thoemi09 for this huge improvement! I pushed some minor changes, and this all looks good now!

@Wentzell Wentzell merged commit 96f63a8 into TRIQS:unstable Apr 24, 2024
4 checks passed
@Thoemi09 Thoemi09 deleted the DEV_DOCUMENTATION branch April 24, 2024 15:12
@hmenke
Copy link
Member

hmenke commented Jun 6, 2024

I'm getting lots of compiler errors when compiling 1.3.0 and it points to this change.

This is the first error, but there are more of the same kind

/home/abuild/rpmbuild/BUILD/triqs-3.3.0/_build/deps/h5_src/c++/h5/array_interface.cpp: In function 'h5::dataspace h5::array_interface::{anonymous}::make_mem_dspace(const h5::array_interface::array_view&)':
/home/abuild/rpmbuild/BUILD/triqs-3.3.0/_build/deps/h5_src/c++/h5/array_interface.cpp:46:77: error: invalid conversion from 'const long long unsigned int*' to 'const hsize_t*' {aka 'const long unsigned int*'} [-fpermissive]
   46 |       dataspace dspace = H5Screate_simple(v.slab.rank(), v.parent_shape.data(), nullptr);
      |                                                          ~~~~~~~~~~~~~~~~~~~^~
      |                                                                             |
      |                                                                             const long long unsigned int*
In file included from /mpcdf/soft/SLE_15/packages/haswell/hdf5/gcc_13-13.1.0/1.14.1/include/H5Ppublic.h:32,
                 from /mpcdf/soft/SLE_15/packages/haswell/hdf5/gcc_13-13.1.0/1.14.1/include/hdf5.h:35,
                 from /home/abuild/rpmbuild/BUILD/triqs-3.3.0/_build/deps/h5_src/c++/h5/array_interface.cpp:26:
/mpcdf/soft/SLE_15/packages/haswell/hdf5/gcc_13-13.1.0/1.14.1/include/H5Spublic.h:303:55: note:   initializing argument 2 of 'hid_t H5Screate_simple(int, const hsize_t*, const hsize_t*)'
  303 | H5_DLL hid_t H5Screate_simple(int rank, const hsize_t dims[], const hsize_t maxdims[]);
      |                                         ~~~~~~~~~~~~~~^~~~~~

My guess is that this doesn't surface in CI, because that still uses GCC 12.

@hmenke
Copy link
Member

hmenke commented Jun 6, 2024

Hm, I'm also getting other errors llike the following. It looks like h5 relies on an old and deprecated API of HDF5.

https://docs.hdfgroup.org/hdf5/v1_14/group___h5_o.html#ga96ce408ffda805210844246904da2842

Deprecated:
As of HDF5-1.12 this function has been deprecated in favor of the function H5Oget_info_by_name2() or the macro H5Oget_info_by_name.

/home/abuild/rpmbuild/BUILD/triqs-3.3.0/_build/deps/h5_src/c++/h5/group.cpp: In function 'herr_t h5::get_group_elements_name_ds(hid_t, const char*, const H5L_info2_t*, void*)':
/home/abuild/rpmbuild/BUILD/triqs-3.3.0/_build/deps/h5_src/c++/h5/group.cpp:167:37: error: too few arguments to function 'herr_t H5Oget_info_by_name3(hid_t, const char*, H5O_info2_t*, unsigned int, hid_t)'
  167 |     herr_t err = H5Oget_info_by_name(loc_id, name, &object_info, H5P_DEFAULT);
      |                                     ^
In file included from /mpcdf/soft/SLE_15/packages/haswell/hdf5/gcc_13-13.1.0/1.14.1/include/H5Apublic.h:21,
                 from /mpcdf/soft/SLE_15/packages/haswell/hdf5/gcc_13-13.1.0/1.14.1/include/hdf5.h:22,
                 from /home/abuild/rpmbuild/BUILD/triqs-3.3.0/_build/deps/h5_src/c++/h5/group.cpp:24:
/mpcdf/soft/SLE_15/packages/haswell/hdf5/gcc_13-13.1.0/1.14.1/include/H5Opublic.h:544:15: note: declared here
  544 | H5_DLL herr_t H5Oget_info_by_name3(hid_t loc_id, const char *name, H5O_info2_t *oinfo, unsigned fields,
      |               ^~~~~~~~~~~~~~~~~~~~
/home/abuild/rpmbuild/BUILD/triqs-3.3.0/_build/deps/h5_src/c++/h5/group.cpp: In function 'herr_t h5::get_group_elements_name_grp(hid_t, const char*, const H5L_info2_t*, void*)':
/home/abuild/rpmbuild/BUILD/triqs-3.3.0/_build/deps/h5_src/c++/h5/group.cpp:175:37: error: too few arguments to function 'herr_t H5Oget_info_by_name3(hid_t, const char*, H5O_info2_t*, unsigned int, hid_t)'
  175 |     herr_t err = H5Oget_info_by_name(loc_id, name, &object_info, H5P_DEFAULT);
      |                                     ^
/mpcdf/soft/SLE_15/packages/haswell/hdf5/gcc_13-13.1.0/1.14.1/include/H5Opublic.h:544:15: note: declared here
  544 | H5_DLL herr_t H5Oget_info_by_name3(hid_t loc_id, const char *name, H5O_info2_t *oinfo, unsigned fields,
      |               ^~~~~~~~~~~~~~~~~~~~
/home/abuild/rpmbuild/BUILD/triqs-3.3.0/_build/deps/h5_src/c++/h5/group.cpp: In function 'herr_t h5::get_group_elements_name_ds_grp(hid_t, const char*, const H5L_info2_t*, void*)':
/home/abuild/rpmbuild/BUILD/triqs-3.3.0/_build/deps/h5_src/c++/h5/group.cpp:183:37: error: too few arguments to function 'herr_t H5Oget_info_by_name3(hid_t, const char*, H5O_info2_t*, unsigned int, hid_t)'
  183 |     herr_t err = H5Oget_info_by_name(loc_id, name, &object_info, H5P_DEFAULT);
      |                                     ^
/mpcdf/soft/SLE_15/packages/haswell/hdf5/gcc_13-13.1.0/1.14.1/include/H5Opublic.h:544:15: note: declared here
  544 | H5_DLL herr_t H5Oget_info_by_name3(hid_t loc_id, const char *name, H5O_info2_t *oinfo, unsigned fields,
      |               ^~~~~~~~~~~~~~~~~~~~

@Thoemi09
Copy link
Contributor Author

Thoemi09 commented Jun 6, 2024

I don't get any errors with gcc 13.2 and and HDF5 1.14.3 but this looks like an HDF5 version problem.

You can try to compile with H5_USE_110_API. This might fix the second issue.

I am not sure about the first issue. We define h5::hsize_t to be uint64_t which should be the same as in the HDF5 header. Could you check in H5Public.h how they define it there?

Anyway, it would definitely be a good idea to get rid of deprecated function calls.

@Thoemi09
Copy link
Contributor Author

Thoemi09 commented Jun 6, 2024

Oh sorry, I just saw that we already define H5_USE_110_API in the h5 CMakeLists.txt file. It should be using H5Oget_info_by_name1 and not H5Oget_info_by_name3.

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 this pull request may close these issues.

3 participants