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.

Sign up for GitHub

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

Jump to bottom

Random-access for variable-based encoding (i.e. use SetStepSelection for ADIOS2 steps) #1706

Merged
merged 33 commits into from
Feb 20, 2025
Merged

Random-access for variable-based encoding (i.e. use SetStepSelection for ADIOS2 steps) #1706

merged 33 commits into from
Feb 20, 2025

Conversation

franzpoeschel
Copy link
Contributor

@franzpoeschel franzpoeschel commented Dec 16, 2024 •
edited
Loading

TODO:

  • MPI-aware preparsing
  • Maybe through out MPI-awareness of preparsing again
  • Edge case: What if a particle species completely vanishes after some iteration, making it appear as an empty, i.e. constant component -> unsupported case, backend will throw
  • Better testing
  • Documentation, examples
  • Replace group-based encoding with variable-based encoding more thoroughly for ADIOS2 (maybe follow-up)
  • Can we make variable-based encoding the default output for ADIOS2? (maybe follow-up for easier reversibility)
  • Check if the engine supports this mode ---> would include HDF5 engine, but it has no first-class support, so setting group-based encoding manually is fine
  • Check if the datasets have the same number of steps as there are snapshots
  • Add sth like "UnselectStepSelection" for reading /rankTable
  • follow-up: maybe pre-parse the entire thing in linear encoding and only then switch to random-access?
  • follow-up: Preparsing also in READ_LINEAR (i.e. eliminate duplicate Iterations better than now)
  • For defaulting v-encoding: Do attributes such as dt or time make sense here at all? ---> Need to preparse those, too. Check all modifiable attributes upon first READ_LINEAR open. --> follow-up, print warning for now
  • Maybe use openPMD 2.0 to introduce a more breaking default for v-based encoding in ADIOS2? --->follow-up
  • Merge ADIOS2: Require v2.9 as minimum #1711 first

@franzpoeschel franzpoeschel added backend: ADIOS2 api: new additions to the API labels Dec 16, 2024
@franzpoeschel franzpoeschel force-pushed the random-access-steps branch 2 times, most recently from 4f7284b to 585b084 Compare December 19, 2024 13:35
@franzpoeschel franzpoeschel force-pushed the random-access-steps branch 5 times, most recently from 238fcd1 to 688ffa9 Compare January 15, 2025 14:23
github-advanced-security[bot]
github-advanced-security bot found potential problems Feb 13, 2025
View reviewed changes
This was referenced Feb 13, 2025
Open
Open
@franzpoeschel franzpoeschel force-pushed the random-access-steps branch 2 times, most recently from e41f041 to 9475398 Compare February 14, 2025 17:06
github-advanced-security[bot]
github-advanced-security bot found potential problems Feb 14, 2025
View reviewed changes
test/CoreTest.cpp
@@ -1499,6 +1500,11 @@
#endif
}

TEST_CASE("automatic_variable_encoding", "[adios2]")

Check notice

Code scanning / CodeQL

Unused static function Note test

Static function C_A_T_C_H_T_E_S_T_54 is unreachable (
autoRegistrar55
Loading
must be removed at the same time)
@franzpoeschel franzpoeschel mentioned this pull request Feb 17, 2025
Open
6 tasks
@ax3l ax3l requested review from ax3l and guj February 18, 2025 17:58
@ax3l ax3l self-assigned this Feb 18, 2025
@ax3l
Copy link
Member

ax3l commented Feb 18, 2025

#1711 merged, ready for rebase :)

@franzpoeschel
Copy link
Contributor Author

#1711 merged, ready for rebase :)

done ;)

@franzpoeschel
Copy link
Contributor Author

Note to self: Benchmark this on a Series with many Iterations, compare with group/file encoding and with variable encoding in linear access

ax3l
ax3l reviewed Feb 18, 2025
View reviewed changes
include/openPMD/IO/IOTask.hpp
@@ -638,8 +672,13 @@ struct OPENPMDAPI_EXPORT Parameter<Operation::ADVANCE>
new Parameter<Operation::ADVANCE>(std::move(*this)));
}

struct StepSelection
{
std::optional<size_t> step;
Copy link
Member

Choose a reason for hiding this comment

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

please include <optional> for stability in this file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, I've also removed an unused <map> import

@franzpoeschel
wip testing
d9d1657
@franzpoeschel
CI fixes
a3cabdd
@franzpoeschel
Fix test
f8a9e19
@franzpoeschel
Add MPI_CHECK
7353439
@franzpoeschel
Stricter warning about group-based encoding in BP5
926581e
@franzpoeschel
variableBased as default in ADIOS2
0da9a01
@franzpoeschel
Add error check for non-homogeneous datasets
432fe2e
@franzpoeschel
Reset steps before reading the rankTable
1c230e1
@franzpoeschel
Use writeIterations() in MPI Benchmark
544e765
@franzpoeschel
Use group tables by default
547546c
@franzpoeschel
Add iterate_nonstreaming_series test to parallel tests
313b9a0 
This covers the use case that the snapshot attribute has more than just
one entry.
@franzpoeschel
Remove std::cout calls
30fe9b3
@franzpoeschel
wip: use variable-based encoding in tests
e535762
@franzpoeschel
CI fixes
185d023
@franzpoeschel
Fix Windows testing
02a4fd4
@franzpoeschel
Cleanup
126664c
@franzpoeschel
Warn unimplemented modifiable attributes for now
182da14
@franzpoeschel
Fix test???
c2e2b2b
@franzpoeschel
Add this to variableBasedSeries test
9c6f4aa
@franzpoeschel
Remove debugging line
1794b3a
@franzpoeschel
Use own flag for the warning
3c95b2c 
Previously implementation did not work: ornladios/ADIOS2#4466
@franzpoeschel
Documentation
dce876e
@franzpoeschel
Skip inhomogeneous datasets instead of outright failing
816ae26
@franzpoeschel
add test for default iteration encoding
29f6403
@franzpoeschel
Move test to its own file
ffd887d
@franzpoeschel
Don't distinguish ADIOS2 v2.9 any more
e15ab8c
@franzpoeschel
Some more documentation
b9158a6
@ax3l ax3l merged commit ff52a8d into openPMD:dev Feb 20, 2025
30 of 31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ax3l ax3l ax3l approved these changes

@guj guj Awaiting requested review from guj

Assignees

@ax3l ax3l

Labels
api: new additions to the API backend: ADIOS2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@franzpoeschel @ax3l