-
Notifications
You must be signed in to change notification settings - Fork 218
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
openPMD Plugin #2966
openPMD Plugin #2966
Conversation
include/picongpu/versionFormat.cpp
Outdated
<< OPENPMDAPI_VERSION_MINOR | ||
<< "." | ||
<< OPENPMDAPI_VERSION_PATCH | ||
<< " (" |
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.
note: OPENPMDAPI_VERSION_LABEL
might be empty and should only be added if it's a non-empty string.
Examples:
- 0.6.2-alpha
- 1.2.3-rc1
- 1.2.3
(see how we build the PIConGPU version string for comparison)
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 changed that part, but is the value actually used anywhere? Everywhere throughout the codebase where getSoftwareVersions( std::ostream & cliText )
is called, the return value is discarded. Only the writes to cliText
have an effect, really.
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.
It's still a bit off: it must be -label
in semver (see example in first comment).
The idea is to include this in openPMD meta info at some point openPMD/openPMD-standard#137 as well as picongpu --version
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.
@franzpoeschel you can now use the new public API to build your version strings:
https://github.com/openPMD/openPMD-api/blob/0.11.0-alpha/include/openPMD/version.hpp#L54-L80
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've changed it to use that now
56ff067
to
02eef50
Compare
include/picongpu/plugins/openPMD/restart/RestartFieldLoader.hpp
Outdated
Show resolved
Hide resolved
0e0c177
to
b756abc
Compare
CI says:
|
@@ -54,7 +54,7 @@ TBG_ipngYX="--i_png.period 10 --i_png.axis yx --i_png.slicePoint 0.5 --i_png.fol | |||
TBG_eBin="--e_energyHistogram.period 100 --e_energyHistogram.filter all --e_energyHistogram.binCount 1024 --e_energyHistogram.minEnergy 0 --e_energyHistogram.maxEnergy 5000" | |||
TBG_iBin="--i_energyHistogram.period 100 --i_energyHistogram.filter all --i_energyHistogram.binCount 1024 --i_energyHistogram.minEnergy 0 --i_energyHistogram.maxEnergy 2000000" | |||
|
|||
TBG_plugins="!TBG_ipngYX \ | |||
TBG_plugins="--openPMD.period 1000 --openPMD.file simData%T.bp !TBG_ipngYX \ |
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 we prefer an underscore separation and zero-prefixed: simData_%06T.bp
!TBG_ipngYX
should be on the next line
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 need to keep an eye on which files I commit, those were supposed to stay local for now.
Should I revert them back to the older versions or should I change them to use the new openPMD plugin? At the moment, this PR does not change the default writing plugin which is still ADIOS.
include/picongpu/versionFormat.cpp
Outdated
<< OPENPMDAPI_VERSION_MINOR | ||
<< "." | ||
<< OPENPMDAPI_VERSION_PATCH | ||
<< " (" |
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.
It's still a bit off: it must be -label
in semver (see example in first comment).
The idea is to include this in openPMD meta info at some point openPMD/openPMD-standard#137 as well as picongpu --version
EDIT: is resolved If a user is absolutely destined to use HDF5 backend until this bug is resolved, the following temporary patch to the openPMD API is suggested: In file_id = H5Fopen(name.c_str(),
flags,
m_fileAccessProperty);
if( file_id < 0 )
throw no_such_file_error("Failed to open HDF5 file " + name); with file_id = H5Fopen(name.c_str(),
flags,
m_fileAccessProperty);
if( file_id < 0 )
file_id = *m_openFileIDs.begin(); This workaround assumes that an error -- if one occurs while opening a file -- is due to the mentioned issue in the openPMD API and then uses the fact that in the openPMD plugin only deals with one file at a time to retrieve the file handle from |
3b3dd63
to
5ed5342
Compare
813c00e
to
157ac20
Compare
.productOfComponents(); | ||
if( size > 0 ) | ||
mThreadParams.fieldBuffer = std::shared_ptr< float_X >{ | ||
new float_X[ size ], |
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.
Forgot to ask during your talk: is there an issue with using std::vector
instead of C-style array? (and potentially .data()
to get a raw pointer to its elements).
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.
This should be no problem. The openPMD API explicitly allows to use contiguous containers and implements this by mocking a shared pointer via .data()
.
Would using a std::vector
be preferrable?
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 like std::vector
a little bit more. Of course, in this case, it is not such a big deal since even with the raw pointers you take care that it is properly deleted.
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, always prefer smart-ptrs and contiguous containers (std::vector/std::array) over C-style arrays.
openPMD-api supports std::array
and std::vector
out-of-the-box for that purpose, as Franz already mentioned :) Here he is using a smart-ptr, which is totally fine as well for temporary memory.
https://github.com/openPMD/openPMD-api/blob/dev/include/openPMD/auxiliary/ShareRaw.hpp
openPMD/openPMD-api#386
https://openpmd-api.readthedocs.io/en/0.9.0-alpha/usage/firstwrite.html#register-chunk
std::shared_ptr
is the implementation detail below, works here as well if you do not want to keep track when to de-allocate it. Actually also a good usage here, if you want to use mThreadParams.fieldBuffer
only until registered for write/read and be done at flush with it. Otherwise its your responsibility when to destruct the container to free its memory.
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.
As mentioned further down the thread, I've moved it to std::vector
due to easier resizing.
a837a14
to
0252c00
Compare
Just one more comment on Franz's note:
Right now, I'm working on the Molecular Dynamics Simulation domain extension of openPMD standard for start-to-end simulation platform. We also met a similar problem. Our preferred style is the Data structure extended example described here. However, the |
Yes, @franzpoeschel (or volunteers forward :) ) please add this functionality to openPMD-api and also open an issue therein. We need a simple low-level API that can just add attributes and records arbitrarily along the "in-file three". That was originally a task when Fabian was still working on this but did not get finished yet. That said: one can totally implement openPMD, including new extensions, without openPMD-api in the meantime for @ejcjason :) But ideally I want to have this in the API of course, because it makes it easier to adopt. |
} | ||
|
||
/* copy species only one time per timestep to the host */ | ||
if( ( mThreadParams.strategy == WriteSpeciesStrategy::ADIOS ) && |
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.
parentheses are not required: should be if( mThreadParams.strategy == WriteSpeciesStrategy::ADIOS && lastSpeciesSyncStep != currentStep )`
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 like to be explicit with parens, but this was maybe a bit very explicit. Removed them.
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.
@franzpoeschel Could you please squash the commit. Never the less from my side all looks good. (only one thing with too much parentheses but it is not critical)
f140eee
to
b6a783e
Compare
Results of running the openPMD validator on the checkpoint files:
The validator consistently complains about The only difference for this attribute between the valid example dataset created by
EDIT: I quickly patched the openPMD API for testing purposes to write double-precision floating points instead of single precision, the validator likes this more:
The validator shouldn't consider single precision an error 1, 2. |
Concerning the two warnings:
|
Without particle patches a restart with PIConGPU should not be possible. |
@franzpoeschel regarding the author. If I got it right, we normally set it automatically via your picongpu profile variable, like this, and then tbg substitutes it for all picongpu runs. |
But the ADIOS plugin currently does restarts without particlePatches? I will look into it tomorrow.
I was launching PIConGPU bare-bones without tbg. I ran another test, this time setting |
Restarts definitely work! This was one of the main concerns and one reason for performing the test I did. Thus the question: Is it really necessary to have these? If not, I don't see a reason to invest more time in this already working backend. |
If it work we should merge it as it is and add it in a separate PR.
Am 6. Mai 2020 14:33:39 MESZ schrieb Klaus Steiniger <notifications@github.com>:
…> > > * Particle patches: these are only written by the HDF5 plugin so
far, not by the ADIOS plugin which I based this plugin on. I would
suggest keeping this for a later PR?
> >
> >
> > Without particle patches a restart with PIConGPU should not be
possible.
> > Is it hard to copy over the particle patches from the HDF5 plugin.
>
> But the ADIOS plugin currently does restarts without particlePatches?
I will look into it tomorrow.
Restarts definitely work! This was one the main concerns and one reason
for performing the test I did. Thus the question: Is it really
necessary to have these? If not, I don't see a reason to invest more
time in this already working backend.
--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#2966 (comment)
|
Last commit c54bbab adds writing of particle patches. These are not yet used for restarting, but instead the plugin uses the |
Create openPMDWriter based on ADIOSWriter (WIP) Adapt CountParticles partially to openPMD Adapt WriteSpecies to openPMD Translate WriteMeta.hpp to openPMD Add function "asStandardVector()" Translate NDScalars.hpp to openPMD Adapt everything to openPMD except for the main file (openPMDWriter.hpp) Adapt openPMDWriter.hpp to openPMD WIP Change management of openPMD Series Further adapt openPMDWriter to openPMD Add openPMD to CMake Add openPMD to version formatting Properly acquaint openPMD plugin with build and help system Make openPMD namespacing explicit Remove adios-specific fields from ThreadParams First successfull run with LaserWakeField example Cleanup Use clang-format Update licensing information Separate basename and filename extension into two separate help parameters Refactor dataset creation Use Mesh::setTimeOffset() template Causes a linker error with the current version of openPMD API, a fix will be merged soon on the dev branch. Clean up some leftovers Use ASCII characters to spell my name Remove unnecessary whitespaces Adapt to removal of pmacc::forward Remove accidentally versioned config file Make checkpoints usable Fix a number of bugs concerning the reading of checkpoints Still problematic is the attribute "timeOffset", currently mitigated by uncommenting the sanity check in the openPMD API. Needs further investigation. Remove CollectFieldsSizes Legacy from ADIOS writer, not necessary in openPMD. Remove ParticleAttributeSize and openPMDCountParticles Legacy from ADIOS Writer Use clang-format Adhere to openPMD requirements before flushing For a given particle species, the openPMD API requires that required records (such as "position", "positionOffset") and their contained components be defined (not necessarily written). Make sure to define all such datasets before issuing the first flush. Maybe open an issue in the openPMD API to allow for a more flexible usage. Fix an indexing bug Eliminate dataset preparation for NDScalars Also fix a bug where particles were named wrongly in checkpoints. Do not write empty particle datasets Treat non-existing datasets as empty in reading Remove prepared datasets Remove WithWindow struct Use transform to enable ADIOS1 compression Remove accidentally versioned files Rename LoadParticleAttributesFromADIOS to LoadParticleAttributesFromOpenPMD Remove traces of the old ADIOS plugin mostly the word "adios" from comments Take copies and not references of openPMD handles Fix autoformatting Require newer openPMD API Also add ADIOS2_ROOT to CMAKE_PREFIX_PATH Add version label to format string only if present Replace typedefs with using Remove further indexing bug in writing particles_info table Cleanup restart Remove dataset preparation Commit 0b50561 reintroduced a reduced form of dataset preparation in order to adhere to requirements (restrictions) of the openPMD API. This workaround results in dummy dataset writes (likely a bug in the openPMD API), hence this commit reverts those changes. The corresponding pull request in the openPMD API to relax this restriction can be found at openPMD/openPMD-api#518. Postpone writing of meta attributes Due to a bug in the ADIOS1 backend of the openPMD API, the first dataset needs to be written before the first flush. This works around the problem by first defining all the fields and particles. Bug report will follow. Resolve counting bug during particle writing Fix whitespaces Separate ADIOS and HDF5 particle writing strategies Allow choosing strategy as runtime parameter Cleanup Fix openPMD version formatting Update examples to use openPMD Refactor passing of filename and extension Reassemble filename and extension upon opening the series. Fix some missing includes Do not skip writing empty datasets See openPMD PR: openPMD/openPMD-api#529 This allows to write empty datasets Remove debugging leftovers Write timeOffset for particles in a standard-compliant way Do not declare zero-sized arrays C++ standard requires that array size evaluate to a value greater than zero. Do not use storeChunk on empty datasets centralize initialization of thread params from config Fix undefined identifier in assert statements Error passes silently in release builds. Pass JSON config to openPMD::Series ctor Do not copy Series object see openPMD/openPMD-api#534 Allow NULL as configuration infix to denote empty string Adapt to changes in pmacc etc. Enable use of group-based layout as well Requires keeping openPMD Series open. Since openPMD currently has no explicit call to close a file, we implement this only for group-based layout for now. Do not use deprecated Series::setSoftwareVersion call Apply commit b797c0a to openPMD backend Formatting in .cfg files Fix an uninitialized value and an indexing bug Implement reviewers' comments concerning CMakeLists Fix some bugs and remove unneeded parameters Some ADIOS-specific parameters have been remove that haven't been implemented anyway and are going to be implemented via JSON. Don't set compression if set to "none". Also, I missed a small part when porting b797c0a to this plugin. Add documentation for openPMD plugin [WIP] Further write documentation Rename data preparation strategies adios -> doubleBuffer hdf5 -> mappedMemory Old names may still be used alternatively. Fix indexing bug for non domain-bound fields Fix indexing order in fields Implement Reviewer's comments Remove workaround for ADIOS1 backend fix attributes Mesh record component positions and unitSI Iteration: time ParticleAttribute unitDimension Implement reviewer's comments (2) Update copyright headers of files changed Add compression to openPMD TBG example Use openPMD::getVersion remove unnecessary parentheses
Not yet used for restarting, instead the custom 'particles_info' table is used. @todo Remove that in future
37a5fb1
to
2135fb6
Compare
I think we should add an |
Agreed, will get to that next. |
Sounds good, so on top of the |
@franzpoeschel I thought we merged the OpenPMD plugin already. Is everything ready to merge? |
Ah, I thought we were still waiting for the next openPMD release. (Also, I missed the this PR's 1-year anniversary last month :D ) |
OK than I will merge it and we will do all future work in a follow up. |
Implement writing to and reading from openPMD.
Todo
For testing purposes, please use this branch of the openPMD API.
As noted below, the HDF5 backend in the openPMD API is currently broken and may not be used in this plugin at the moment.
The resulting storage files are incompatible with the files written by the existing plugins: The existing plugin writes datasets
/data/<iteration>/picongpu/idProvider/startID
, but the openPMD API only supports writing datasets/data/<iteration>/(fields|particles)/*/*(/*)
at the moment, so those datasets have been temporarily relocated to/data/0/fields/picongpu_idProvider/startId
until the openPMD allows to write "heavy" metadata more flexibly.I've written up a simple guide for building and installing.