-
Notifications
You must be signed in to change notification settings - Fork 51
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
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 Note test
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 Note test
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.
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
include/openPMD/RecordComponent.tpp
Outdated
@@ -397,7 +376,7 @@ | |||
} // namespace detail | |||
|
|||
template <typename Visitor, typename... Args> | |||
constexpr auto RecordComponent::visit(Args &&...args) | |||
auto RecordComponent::visit(Args &&...args) |
Check notice
Code scanning / CodeQL
Unused local variable Note
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
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