-
Notifications
You must be signed in to change notification settings - Fork 53
Joined arrays in ADIOS2 #1382
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
Joined arrays in ADIOS2 #1382
Conversation
d0d1e7a to
4d68621
Compare
4d68621 to
1f299b6
Compare
|
Except for the documentation, this is now somewhat ready. |
| */ | ||
| for (size_t i = 0; i < length_of_patch * patches_per_rank; ++i) | ||
| { | ||
| REQUIRE(float(i) == particleData.get()[i]); |
Check notice
Code scanning / CodeQL
Equality test on floating-point values
| for (size_t i = 0; i < patches_per_rank; ++i) | ||
| { | ||
| REQUIRE(length_of_patch * i == numParticlesOffset.get()[i]); | ||
| REQUIRE(type(length_of_patch * i) == patchOffset.get()[i]); |
Check notice
Code scanning / CodeQL
Equality test on floating-point values
test/ParallelIOTest.cpp
Outdated
| } | ||
| } | ||
|
|
||
| TEST_CASE("joined_dim", "[serial]") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you mean "parallel"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, good catch
test/ParallelIOTest.cpp
Outdated
| auto patchOffset = it.particles["e"].particlePatches["offset"]["x"]; | ||
| auto patchExtent = it.particles["e"].particlePatches["extent"]["x"]; | ||
| Dataset particlePatchesDS(determineDatatype<float>(), {0}); | ||
| particlePatchesDS.joinedDimension = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is a sanity check needed here? do we want to throw error if someone set an invalid value of joinedDimension?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will probably switch to the same API as ADIOS2, i.e. have some magic number JOINED_DIMENSION=std::numeric_limits<>::max and then use something like Dataset ds(<type>, {JOINED_DIMENSION, 128 , 128}). We should add a sanity check there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The constructor of Dataset now verifies this.
franzpoeschel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see inline comments
6a53c77 to
5bdbcc2
Compare
5bdbcc2 to
249ede8
Compare
a941b31 to
92312a7
Compare
92312a7 to
36f81a2
Compare
include/openPMD/RecordComponent.hpp
Outdated
| template <typename T> | ||
| void verifyChunk(Offset const &, Extent const &) const; | ||
|
|
||
| void verifyChunk(Datatype, Offset const &, Extent const &) const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Earlier versions of this PR added additional calls to RecordComponent::storeChunk, so instead of duplicating the verification logic, I extracted it.
Now no longer necessary, but it's still a useful refactoring.
36f81a2 to
64b9a73
Compare
5cd4760 to
8a9968e
Compare
8a9968e to
672985a
Compare
b70a5a1 to
4e4edea
Compare
4e4edea to
200cc69
Compare
23ff8e6 to
c13c20e
Compare
|
This looks great! Potentially can you add a Python test, to make sure the new enum exposed to the bindings works well? |
793576d to
ce51f2a
Compare
d9c545c to
342337e
Compare
Use magic number instead of API call (impl)
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
09eff2d to
32dbc60
Compare
ax3l
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!!
|
|
||
| (Currently) only supported in ADIOS2 no older than v2.9.0 under the conditions listed in the `ADIOS2 documentation on joined arrays <https://adios2.readthedocs.io/en/latest/components/components.html#shapes>`_. | ||
|
|
||
| In some cases, the concrete chunk within a dataset does not matter and the computation of indexes is a needless computational and mental overhead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| In some cases, the concrete chunk within a dataset does not matter and the computation of indexes is a needless computational and mental overhead. | |
| In some cases, the concrete chunk within a dataset does not matter and the computation of indexes is a needless computational, (MPI) communication and mental overhead. |
| if USE_JOINED_DIMENSION: | ||
| # explicit API | ||
| # mymesh.store_chunk(local_data, [], [10, 300]) | ||
| mymesh[:, :] = local_data | ||
| # or short: | ||
| # mymesh[()] = local_data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fancy!
Good examples even in the commented code.
This implements ornladios/ADIOS2#3452 in openPMD and (partially) fixes #1374.
We will need a fallback implementation for backends other than ADIOS2. Quoting the discussion in #1374:
TODO
Needs a standard update: numParticlesOffset may optionally be skipped, particlePatches must then be in order so they can be reconstructed from numParticlesNot updating the standard yet