Characterise what the HDF5 export writes - #1998
Merged
Merged
Conversation
ASCIIExporter has no tests, and its HDF5 writing is the least obvious part of it: raw
simulation data is reordered by slice extraction before it lands in the file, and the
dataset's name and shape both depend on what was asked for. Nothing recorded what a user
actually downloads.
Pin the three shapes SliceHelper.populate produces, a timepoint at a time:
whole volume DataValues (XYZT) {outer=sizeY, inner=sizeX, sizeZ, time}
single slice DataValues (XYT) {sizeY, sizeX, time}
membrane DataValues (MT) {len, time}, written verbatim
A single slice drops Z from the shape and from the dataset name, so it is a different
dataset from a whole-volume export rather than a one-slice version of it. That is easy to
get wrong and impossible to notice without a test.
The membrane case needed a mesh reporting membrane elements. Those only come from the
Geometry based factory with a generated surface collection, which is far more machinery
than a test of "writes the array unchanged" should need, so CartesianMeshTestSupport sets
the count from inside the mesh's own package. Worth knowing while reading it: the type is
inferred from data length and VOLUME is checked first, and a zero length array still
matches MEMBRANE -- so a mesh with no membranes lets such a test pass while measuring
nothing. The assertions refuse that.
Written against the existing native hdf.hdf5lib binding and read back with jhdf, so this
describes today's behaviour and stands on its own. It is also the reference for moving
this writing to jhdf (jhdf#654), which is why it exists now.
SliceHelper becomes package private so a test can reach it. No other production change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ASCIIExporterhas no tests. Its HDF5 writing is the least obvious part of it — raw simulation data is reordered by slice extraction before it lands in the file, and both the dataset's name and its shape depend on what was asked for — so nothing recorded what a user actually downloads.This pins the three shapes
SliceHelper.populateproduces, one timepoint at a time:DataValues (XYZT){outer=sizeY, inner=sizeX, sizeZ, time}DataValues (XYT){sizeY, sizeX, time}DataValues (MT){len, time}, verbatimThe single-slice case is the one worth having in writing: it drops Z from the shape and from the dataset name, so it is a different dataset from a whole-volume export rather than a one-slice version of it.
Two things found while writing it
Membrane data is classified by length, and a mesh with no membranes passes vacuously.
getVariableTypeFromLengthchecks VOLUME first, then MEMBRANE — and a zero length array still matchesgetDataLength(MEMBRANE) == 0.createSimpleCartesianMeshnever populatesmembraneElements(only theGeometrybased factory does, via a generatedSurfaceCollection), so the obvious test would have written a{0, timeCount}dataset and passed while measuring nothing. The first version of this test caught exactly that, and the assertions now refuse it.Rather than build a full geometry with surface generation for a code path that writes the array unchanged,
CartesianMeshTestSupportsets the element count from inside the mesh's own package — the field isprotected, so it is plain package access, no reflection — and says why the elements themselves are left null.The test mesh has two subvolumes. It made no difference to the count in the end, but a uniform image has no membrane surface at all, and the mesh should at least describe something a membrane could exist between.
Scope
hdf.hdf5libbinding and read back with jhdf, so it describes today's behaviour and does not depend on anything unreleased.SliceHelpergoes fromprivateto package private so a test can reach it. That is the only production change — 3 lines, and the file's CRLF line endings are preserved.This exists because that writing is being moved off the native binding to jhdf (upstream jhdf#654, PR jamesmudd/jhdf#865), and rewriting an untested export path on the strength of "it still compiles" is not a thing worth doing. But the characterisation is useful whether or not that lands, which is why it is on its own.
🤖 Generated with Claude Code
https://claude.ai/code/session_01HYdenYzMw35USHDQEATGVq