Add OpenPMD support - #1050
Conversation
Ah, ok. I wasn't reading this correctly in my VisIt plugin. Fixed now, thanks. |
BenWibking
left a comment
There was a problem hiding this comment.
Works in my production runs. Throughly audited with Codex. Did not attempt to manually read each line.
|
ping @Yurlungur for review. |
Sorry I know I'm the blocker here... I'm out sick currently. Will review ASAP |
Yurlungur
left a comment
There was a problem hiding this comment.
Long time coming, and pretty clean in the end. Glad to see you got to reuse some of the utilities from HDF5. Sorry for the long delay in review. I have some comments but I don't consider any of them blocking.
| // Current unclear how to properly handle other vectors and tensors, so everything | ||
| // that not's a proper vector is a a scalar for now. |
There was a problem hiding this comment.
@pgrete what's the status of this? How does this work right now for, e.g., a tensor field?
lroberts36
left a comment
There was a problem hiding this comment.
Awesome! I am glad to see this done. Just a few small non-blocking comments.
| // TODO(?) in principle, we could abstract this to a more general WriteAttributes place | ||
| // and reuse for hdf5 and OpenPMD output with corresponing calls |
There was a problem hiding this comment.
This would be great (WriteOutputFile is pretty big for both OpenPMD and HDF5), but certainly not critical for this PR.
| //---------------------------------------------------------------------------------------- | ||
| //! \fn void OpenPMDOutput:::WriteOutputFile(Mesh *pm) | ||
| // \brief Write output in OpenPMD format | ||
| template <bool WRITE_SINGLE_PRECISION> |
There was a problem hiding this comment.
Why not just template on the type itself (i.e. template <class OutT>)?
There was a problem hiding this comment.
I'm not sure I follow.
| RestartReaderOPMD::SparseInfo RestartReaderOPMD::GetSparseInfo() const { | ||
| SparseInfo info; | ||
| // Only read if data exists. Otherwise return default constructed. | ||
| if (it->containsAttribute("SparseInfo")) { | ||
| auto sinfo_vec = it->getAttribute("SparseInfo").get<std::vector<int8_t>>(); | ||
| info.labels = it->getAttribute("SparseFields").get<std::vector<std::string>>(); | ||
| info.num_sparse = static_cast<int>(info.labels.size()); | ||
| info.num_blocks = sinfo_vec.size() / info.num_sparse; | ||
| info.dealloc_count = it->getAttribute("SparseDeallocCount").get<std::vector<int>>(); | ||
|
|
||
| // copy "vector" data to bool pointer for compatiblity between output backends | ||
| info.allocated.reset(new bool[sinfo_vec.size()]); | ||
| for (int i = 0; i < sinfo_vec.size(); i++) { | ||
| info.allocated[i] = sinfo_vec.at(i); | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't know what the solution is, but it concerns me a little that we basically have to mirror all of the parameter reading for HDF5 and OpenPMD exactly to ensure exact restart behavior.
There was a problem hiding this comment.
I think the solution would be (along the lines of "simplify"/write common WriteOutputFile) to have an abstraction to write arbitrary attributes so that a restart output would just call WriteAttribute that under the hoods then calls the backend specific implementation.
| if (PARTHENON_ENABLE_OPENPMD) | ||
|
|
||
| # h5py is also needed for the current test | ||
| list(APPEND REQUIRED_PYTHON_MODULES openpmd_api h5py) | ||
|
|
||
| # Restart | ||
| list(APPEND TEST_DIRS restart_opmd) | ||
| list(APPEND TEST_PROCS ${NUM_MPI_PROC_TESTING}) | ||
| list(APPEND TEST_ARGS "--driver ${PROJECT_BINARY_DIR}/example/advection/advection-example \ | ||
| --driver_input ${CMAKE_CURRENT_SOURCE_DIR}/test_suites/restart_opmd/parthinput.restart \ | ||
| --num_steps 3") | ||
| list(APPEND EXTRA_TEST_LABELS "") | ||
|
|
||
| endif() | ||
|
|
There was a problem hiding this comment.
Should this just be part of the restart test? I think just adding OpenPMD restart output and restarting from both OpenPMD and HDF5 would test the same things and maybe run a little quicker. (I assume this new test takes as long as the current restart test).
There was a problem hiding this comment.
In principle yes, but until OpenPMD is on by default (and more broadly used) I think it makes sense to keep this separate because the logic is simpler in two separate tests than having a configure dependent number of steps and test logic for one (large) test.
|
Are people happy with the latest changes? I think we'd be good to go then. |
|
Ship it. |
PR Summary
Leftover todos (potentially for future PRs):
For upcoming PRs:
Metadata::NoneMetadata::FineMetadata::CoordinatesVecPR Checklist