Output XDMF that conforms to spec #820
Conversation
|
I've tested this on the outputs from the advection example with Paraview 5.11.0 and it works fine. With the same output files, VisIt 3.22 still crashes. |
|
The crashes are unrelated, and are fixed by #796. |
|
Visit works, but Paraview is still unhappy (emits lots of identical warnings). |
|
Ah ok. |
|
Ok, it works for me now in both VisIt and Paraview without any warnings. @Yurlungur @brryan does this look good to you? |
|
I tried this with both the advection example outputs and a problem in AthenaPK. I think (?) that covers both scalar and vector output, but I didn't test a single block simulation. |
Good enough for me. |
src/outputs/parthenon_hdf5.cpp
Outdated
| @@ -264,11 +264,12 @@ static std::string stringXdmfArrayRef(const std::string &prefix, | |||
| const std::string &hdfPath, | |||
| const std::string &label, const hsize_t *dims, | |||
| const int &ndims, const std::string &theType, | |||
| const int &precision) { | |||
| const int &precision, bool isVector) { | |||
There was a problem hiding this comment.
Why this change?
isVector does not seem to be used.
There was a problem hiding this comment.
Yes this was a mistake, thanks for noticing this
| // TODO(JL) 3 and 5 are literals here, careful if they change. | ||
| // "3" rows for START, STRIDE, and COUNT for each slab with "5" (H5_NDIM) entries. | ||
| // START: iblock variable(_component) 0 0 0 | ||
| // STRIDE: 1 1 1 1 1 | ||
| // COUNT: 1 vector_size nx3 nx2 nx1 |
There was a problem hiding this comment.
I think the comment is still valid.
src/outputs/parthenon_hdf5.cpp
Outdated
| for (int i = 2; i < ndims; i++) { | ||
| fid << dims[i] << " "; | ||
| } |
There was a problem hiding this comment.
Why this change (compared to using dims321)?
I think we should remain consistent, i.e., either switch to a loop everywhere and get rid of dims321 or use dims321 wherever possible (which should make future changes easier as all places can be identified by the same pattern)
There was a problem hiding this comment.
No reason now for this change, and in fact it was overly specialized to the 1D tensor dimension case -- reverted. I agree about dims321 consistency.
src/outputs/parthenon_hdf5.cpp
Outdated
| for (const auto &vinfo : var_list) { | ||
| // TODO(BRR) just let vinfo provide this |
There was a problem hiding this comment.
@brryan is that comment still valid (as vinfo is already used)?
There was a problem hiding this comment.
I had thought about storing alldims itself in vinfo but I don't think that's a meaningful change -- comment removed
Co-authored-by: Philipp Grete <pgrete@hs.uni-hamburg.de>
Co-authored-by: Philipp Grete <pgrete@hs.uni-hamburg.de>
Thanks for pinging me on this @BenWibking, yes I am responsible for these issues :). I made the fixes in response to @pgrete's comments in #796. While looking at this I noticed that scalar variable output wasn't working with XDMF, e.g. the Since the changes I made weren't entirely trivial could I trouble you to merge in my latest |
I've checked that |
|
I removed the unused parameter, formatted the code, and added a changelog. |
Thanks @BenWibking!
Thanks @pgrete! (sorry I missed the unused parameter) |
|
The Python/C++ formatting static check does not seem to be triggering. I tried enabling/disabling auto-merge to no effect. Is something wrong? |
|
Looks like the format check was skipped for some reason. |
|
I cloned the branch and ran formatting manually. It passes, so I'm going to force merge. @pgrete I think there maybe some extra step needed to get the formatting workflow to work on forks? |
PR Summary
This changes the XDMF output to conform to the current XDMF specification (available here: https://gitlab.kitware.com/xdmf/xdmf/blob/master/Xdmf.dtd).
To fix the data format issues, this also incorporates the changes proposed in #796.
Fixes #819.
PR Checklist