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

New API to allow storing only stats for primary variables #4328

Merged
merged 8 commits into from
Sep 17, 2024

Conversation

anagainaru
Copy link
Contributor

@anagainaru anagainaru commented Aug 28, 2024

Add a bool in VariableBase to control what we write and corresponding APIs for CXX, C, Python, Fortran.

CXX API

varI32.StoreStatsOnly(true);

C bindings:

adios2_store_stats_only(varI32, adios2_true);

Fortran bindings:

call adios2_store_stats_only(varI32, .true., ierr)

Python bindings

varI32.StoreStatsOnly(True)

The reader can inquire variables and get stats about each block but will not be able to read data. A std::runtime_error exception will be thrown if data is read for a StatsOnly variable.

$ ./bin/bpls -ld ADIOS2BPWriteReadStatsOnly.bp
  float    r32   {8} = 1.1 / 8.1

bpls caught an exception
No data exists for this variable

@anagainaru anagainaru force-pushed the statsOnlyMode branch 6 times, most recently from 034ad0e to 6b5a5a9 Compare August 30, 2024 01:06
@anagainaru anagainaru marked this pull request as ready for review August 30, 2024 01:14
@anagainaru anagainaru changed the title [WIP] New API to allow storing only stats for primary variables New API to allow storing only stats for primary variables Aug 30, 2024
@anagainaru
Copy link
Contributor Author

Ready for review

Copy link
Contributor

@pnorbert pnorbert left a comment

Choose a reason for hiding this comment

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

I feel like the function name does not convey what it is about. How about an inverse approach, like SetStoreMetadataOnly(bool)?

One thing is missing though, the new python API in python/adios2/variable.py. New tests should just use that, which will exercise the adios2.bindings indirectly as well.

@anagainaru anagainaru force-pushed the statsOnlyMode branch 3 times, most recently from 810bb5c to e26d709 Compare August 30, 2024 15:54
@anagainaru
Copy link
Contributor Author

@pnorbert this is ready for review.

@vicentebolea
Copy link
Collaborator

Just an idea: sometime feels better to have storeMetadataOnlyOn() storeMetadataOnlyOff()

@anagainaru
Copy link
Contributor Author

Just an idea: sometime feels better to have storeMetadataOnlyOn() storeMetadataOnlyOff()

Normally I would agree, but in this case I don't want users to be able to turn on and off. We either store only stats or we store everything. We talked about the API and decided we do not want to change the DefineVariable call to add this option (the cleanest choice), but rather include a new function.

@pnorbert pnorbert merged commit 532747a into ornladios:master Sep 17, 2024
38 checks passed
@pnorbert
Copy link
Contributor

Just an idea: sometime feels better to have storeMetadataOnlyOn() storeMetadataOnlyOff()

Normally I would agree, but in this case I don't want users to be able to turn on and off. We either store only stats or we store everything. We talked about the API and decided we do not want to change the DefineVariable call to add this option (the cleanest choice), but rather include a new function.

I believe he meant the same functionality just changing syntax from having a bool argument to separate functions. Graphics packages are full of these. I don't feel better, however, seeing that in adios as a first.

@anagainaru anagainaru deleted the statsOnlyMode branch September 17, 2024 11:56
@anagainaru anagainaru added this to the v2.11.0 milestone Oct 1, 2024
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