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

decl_hdf5 MPI-I/O Collective or Independent, fix #419 #502

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

jmorice91
Copy link
Contributor

@jmorice91 jmorice91 commented Nov 27, 2024

Possibility to choose parallel MPIIO pointer (COLLECTIVE/INDEPENDENT) for hdf5 in parallel.

List of things to check before making a PR

Before merging your code, please check the following:

  • you have added a line describing your changes to the Changelog;
  • you have added unit tests for any new or improved feature;
  • In case you updated dependencies, you have checked pdi/docs/CheckList.md
  • you have checked your code format:
    • you have checked that you respect all conventions specified in CONTRIBUTING.md;
    • you have checked that the indentation and formatting conforms to the .clang-format;
    • you have documented with doxygen any new or changed function / class;
  • you have correctly updated the copyright headers:
    • your institution is in the copyright header of every file you (substantially) modified;
    • you have checked that the end-year of the copyright there is the current one;
  • you have updated the AUTHORS file:
    • you have added yourself to the AUTHORS file;
    • if this is a new contribution, you have added it to the AUTHORS file;
  • you have added everything to the user documentation:
    • any new CMake configuration option;
    • any change in the yaml config;
    • any change to the public or plugin API;
    • any other new or changed user-facing feature;
    • any change to the dependencies;
  • you have correctly linked your MR to one or more issues:
    • your MR solves an identified issue;
    • your commit contain the Fix #issue keyword to autoclose the issue when merged.

@jmorice91 jmorice91 changed the title Mpio pointer decl_hdf5 MPI-I/O Collective or Independent, fix #419 Nov 27, 2024
@jmorice91 jmorice91 self-assigned this Nov 27, 2024
@jmorice91
Copy link
Contributor Author

Previous review are in #494

Yushan-Wang
Yushan-Wang previously approved these changes Nov 28, 2024
JAuriac
JAuriac previously approved these changes Dec 2, 2024
Copy link

@JAuriac JAuriac left a comment

Choose a reason for hiding this comment

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

OK.
Three minor comments to change.

@jmorice91 jmorice91 dismissed stale reviews from JAuriac and Yushan-Wang via 53d0076 December 4, 2024 13:42
{
Raii_hid xfer_lst = make_raii_hid(H5Pcreate(H5P_DATASET_XFER), H5Pclose);
if (use_mpio) {
if (0 > H5Pset_dxpl_mpio(xfer_lst, m_mpio)) handle_hdf5_err();
Copy link
Member

Choose a reason for hiding this comment

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

My personal opinion: I think that all if/else blocks should use curly braces, even if there is only 1 line.

Suggested change
if (0 > H5Pset_dxpl_mpio(xfer_lst, m_mpio)) handle_hdf5_err();
if (0 > H5Pset_dxpl_mpio(xfer_lst, m_mpio)) {
handle_hdf5_err();
}

Comment on lines 220 to 222
if (m_direction == READ)
do_read(ctx, h5_file, xfer_lst);
else
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (m_direction == READ)
do_read(ctx, h5_file, xfer_lst);
else
if (m_direction == READ) {
do_read(ctx, h5_file, xfer_lst);
} else {

@@ -328,7 +340,7 @@ hid_t Dataset_op::dataset_creation_plist(Context& ctx, const Datatype* dataset_t
return dset_plist;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return dset_plist;
return dset_plist;
}

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