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

HDF5: Explicit control over chunking #1591

Merged
merged 9 commits into from
Feb 26, 2024

Conversation

franzpoeschel
Copy link
Contributor

@franzpoeschel franzpoeschel commented Jan 24, 2024

Series("output.h5", Access::CREATE, R"(hdf5.dataset.chunks = "auto")");
// ...
Dataset dataset = Dataset(datatype, global_extent, R"(
[hdf5.dataset]
chunks = [10, 100]
)");
record_component.resetDataset(dataset);

TODO:

@ax3l ax3l requested review from ax3l, guj and jeanbez January 30, 2024 18:37
@guj
Copy link
Contributor

guj commented Jan 31, 2024

I see there are two styles of json for series and dataset

  • hdf5.dataset.chunks = "auto"
  • [hdf5.dataset] chunks = [10, 100]

any chance to consolidate?

@franzpoeschel
Copy link
Contributor Author

I see there are two styles of json for series and dataset

* hdf5.dataset.chunks = "auto"

* [hdf5.dataset] chunks = [10, 100]

any chance to consolidate?

That is TOML, and both forms are equivalent in TOML. The JSON form would be {"hdf5":{"dataset":{"chunks": "..."}}}.
TOML does not have a canonical form on purpose, you can write either of the above as the other as well.

@guj
Copy link
Contributor

guj commented Jan 31, 2024

Oh, I see. Was wondering.

That is TOML, and both forms are equivalent in TOML. The JSON form would be {"hdf5":{"dataset":{"chunks": "..."}}}. TOML does not have a canonical form on purpose, you can write either of the above as the other as well.

Comment on lines 669 to 671
//! @todo allow overwrite with user-provided chunk size
// for( auto const& val : parameters.chunkSize )
// chunk_dims.push_back(static_cast< hsize_t >(val));
Copy link
Member

Choose a reason for hiding this comment

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

@franzpoeschel can this todo now be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, this todo is now fixed

Copy link
Member

@ax3l ax3l left a comment

Choose a reason for hiding this comment

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

Looks great!

Please see last inline comment and then feel free to merge :)

@ax3l ax3l self-assigned this Feb 26, 2024
@franzpoeschel
Copy link
Contributor Author

Looks great!

Please see last inline comment and then feel free to merge :)

Thanks! I removed the old todos

@franzpoeschel franzpoeschel enabled auto-merge (squash) February 26, 2024 19:26
@franzpoeschel franzpoeschel merged commit 30e5bde into openPMD:dev Feb 26, 2024
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants