TEST: the regression guards are named for what they guard - #56
Merged
Conversation
The tests standing guard over already-fixed bugs were scattered and generically named, so nothing tied a test to the behaviour it pins down and a refactor could quietly undo one. Each is now named for what it asserts. The numeric prefixes those names carried are gone with them, in the test names and in the section banners and docstrings alike: they were opaque to anyone reading this suite and pointed nowhere a reader of it can follow. Two gaps the audit turned up, both already immune and kept as guards: a Lagrange cell type reads rather than raising, and a write holding several element types carries its per-element data through. The Lagrange guard now covers all seven of the high-order codes at the node count each shape admits, and sends the type back out again so the reverse table is pinned too. Two groups that both claim one element cannot share a physical tag, so that write is guarded for the warning it owes rather than for a round trip the format cannot give.
skoudoro
force-pushed
the
p4/named-regression-tests
branch
from
September 5, 2026 22:15
a14a699 to
5ba8fc1
Compare
skoudoro
force-pushed
the
p4/named-regression-tests
branch
3 times, most recently
from
September 8, 2026 03:20
01a0c6f to
5ecf723
Compare
A legacy VTK SCALARS header names between one and four components and no more. The writer spelled every tuple it had no VECTORS or TENSORS branch for as SCALARS whatever its width, so a nine-component per-element array went out as "SCALARS name double 9" - a count outside the range the format gives that field, which leaves what the file means to how forgiving the reader that opens it happens to be. Such a tuple now travels as a FIELD array, whose own header carries the component count and so has no ceiling; four components and under keep the SCALARS spelling they had, with the lookup table that comes with it. An array of no components at all is outside the same range from the other end, and is dropped with a warning before its section header is written rather than spelled "SCALARS name double 0" - a header VTK's own reader stops on. The components are counted off the value's shape rather than off an attribute of it, so a caller keying an attribute by a nested list is answered where the writer already took either. A binary block is closed by a newline, and the keyword line after it is found by reading to the next one. Only the dataset's FIELD block wrote that newline: the points, the cells, the cell types and every attribute array ran straight into the keyword after them, so the terminator a reader found was the first 0x0a inside the following payload and the keyword line it read began in the middle of the numbers - sections a reader can only find by hunting for the keyword, as this one did. Every binary block now closes the way the format says, and a file written before this still reads: the newline is stepped over where it is there rather than required. One writer owns the terminator, so no block can be written without it. A symmetric tensor's six components are XX, YY, ZZ, XY, YZ, XZ - the order VTK itself hands back for the cell at (i, j). The writer mirrored a six-component array into the full 3x3 a TENSORS section holds using the classical Voigt order instead, whose last three run the other way, so XZ and YZ came out in each other's places. Nothing downstream can tell that from a tensor that was always that way, which is why it went unseen. A TENSORS6 section is read as the six components it holds. VTK 9 writes a symmetric tensor with that keyword, and every reader here matched it as a TENSORS and took its tuples for nine: an ASCII file failed with a CodecError about a row that is not numbers, and a binary one read six values plus whatever followed them, then went on from the middle of the next array and dropped every array after it. The second number on a v4.2 CELLS line counts the values that follow it, and was taken from the length of the whole connectivity rather than from the run the offsets name. The two are the same for a mesh built here, but one whose last offset stops short of its connectivity - a slice kept at its original length - declared values the section never wrote, and the spelled and the binary spelling disagreed about it besides. Both now declare the run they go on to write. The first number on a v5.1 CELLS line counts the offsets that follow it, and every offset past the first names a cell. The whole offsets array went out whatever the mesh's cell count, so a mesh holding one kept at a buffer's original length declared cells the CELL_TYPES after it never named - and a reader that believes the offsets builds them anyway, taking each one's type from past the end of the array that holds it. VTK's own reader answered such a file with three cells where this one read two. Both sections are now cut to the cells the types name, and an offsets array beginning partway into the connectivity is rebased onto the run written after it, so the two versions answer that mesh with the same cells rather than only v4.2 doing so. An attribute array whose row count is not the one its section covers is dropped, with a warning, before that section is declared. Only the section header says where one of its arrays ends, so an array of more rows ran past that end and one of fewer stopped short of it, and either way the reader took the values on the wrong side of the boundary for the keyword line that should have been there - costing every array after it in the section as well. VTK's own reader stopped on such a file with "Unsupported cell attribute type". No block is built whole before it is written. A run of cells that share a node count - every mesh of one element type, and any mixed one whose types happen to agree - is laid out as rows of an array rather than a cell at a time, and the spelled and binary spellings share that layout; so do the ragged rows a mixed mesh spells, which take a row at a time off one shared iterator rather than slicing the run per cell. CELL_TYPES is translated by indexing a table built once at import rather than by a dictionary hop per cell, and nothing is sized by the codes themselves: a code no table names costs a pass rather than an allocation the size of the code, and reads out as the polygon fallback the per-code lookup already gave it. Writing 400k triangles: spelled, 923 ms and 54 MB above the mesh became 431 ms and 6 MB; binary, 564 ms and 128 MB became 15 ms and 10 MB. The bytes are unchanged but for one: a SCALARS block of two to four components puts a tuple to a line where it ran them all onto one, which is the layout every other multi-component section already had. The regression guards that stood over already-fixed bugs are named for what they guard rather than by number, and the numeric prefixes are gone from the section banners and docstrings with them. Two gaps the audit turned up, both already immune and kept as guards: a Lagrange cell type reads rather than raising, and a write holding several element types carries its per-element data through. The guard that holds the writer to a reader other than this one now asks VTK's own, which is what defines the format, and fails on a message to its error window rather than only on a grid it could not build. That window is process-global, so it is put back on the way out, and a file VTK does refuse is read through it first: the reader is quiet about more than it rejects, and silence only counts once the window is known to be live. VTK is never a dependency - it has no buildable sdist, so declaring it would break an editable install wherever no wheel is published - and is installed by a CI job of its own instead, which is where those guards run rather than skip.
skoudoro
force-pushed
the
p4/named-regression-tests
branch
from
September 8, 2026 15:55
5ecf723 to
e086cf5
Compare
Collaborator
Author
|
all good, merging |
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.
The tests standing guard over already-fixed bugs were scattered and generically
named, so nothing tied a test to the behaviour it pins down and a refactor could
quietly undo one. Each is now named for what it asserts.
The numeric prefixes those names carried are gone with them, in the test names
and in the section banners and docstrings alike: they were opaque to anyone
reading this suite and pointed nowhere a reader of it can follow. No test was
lost to the rename — the suite collects 2562 where it collected 2510, the new
ones being the guards below.
Two gaps the audit turned up, both already immune and kept as guards: a Lagrange
cell type reads rather than raising, and a write holding several element types
carries its per-element data through. The Lagrange guard covers all seven
high-order codes at the node count each shape admits and sends the type back out
so the reverse table is pinned too. Two groups that both claim one element
cannot share a physical tag, so that write is guarded for the warning it owes
rather than for a round trip the format cannot give.
The bugs the audit turned up
A
SCALARSheader names between one and four components and no more. Thewriter spelled every tuple it had no
VECTORSorTENSORSbranch for asSCALARSwhatever its width, so a nine-component per-element array went out asSCALARS name double 9— past what the format admits, which leaves what thefile means to how forgiving the reader that opens it happens to be. Such a tuple
now travels as a
FIELDarray, whose own header carries the component count andso has no ceiling; four components and under keep the
SCALARSspelling and itslookup table. An array of no components is outside the same range from the other
end and is dropped with a warning before its section header is written — that
one VTK's own reader does stop on, with
Cannot read scalar header!.A binary block is closed by a newline. Only the dataset's
FIELDblockwrote one. The points, the cells, the cell types and every attribute array ran
straight into the keyword after them, so the terminator a reader finds is the
first
0x0ainside the following payload — sections a reader can only find byhunting for the keyword, as this one did. A file written before this still
reads: the newline is stepped over where it is there rather than required. One
writer now owns the terminator, so no block can be written without it.
A symmetric tensor's six components are
XX,YY,ZZ,XY,YZ,XZ.The writer mirrored a six-component array into the full 3x3 using the classical
Voigt order instead, whose last three run the other way, so
XZandYZcameout in each other's places. This changes the bytes written for every existing
six-component array — nothing downstream could tell the old ones from a tensor
that was always that way, which is why it went unseen.
A
TENSORS6section is read as the six components it holds. VTK 9 writes asymmetric tensor with that keyword; every reader here matched it as a
TENSORSand took its tuples for nine. An ASCII file failed with a
CodecErrorabout arow that is not numbers; a binary one read six values plus whatever followed
them, then went on from the middle of the next array and dropped every array
after it.
A v4.2
CELLSline declares the run its offsets name. The second numbercounts the values that follow it and was taken from the length of the whole
connectivity. The two agree for a mesh built here, but one whose last offset
stops short of its connectivity declared values the section never wrote — and
the spelled and binary spellings disagreed about it besides.
A v5.1
CELLSsection is cut to the cellsCELL_TYPESnames. The firstnumber counts the offsets that follow it and every offset past the first names a
cell, but the whole offsets array went out whatever the mesh's cell count. A
mesh holding one kept at a buffer's original length declared cells the
CELL_TYPESafter it never named, and a reader that believes the offsets buildsthem anyway, taking each one's type from past the end of the array that holds
it: VTK's own reader answered such a file with three cells where this one read
two. An offsets array beginning partway into the connectivity is rebased onto
the run written after it, so the two versions now answer that mesh with the same
cells rather than only v4.2 doing so.
An attribute the section does not cover is dropped before its header. Only
the section header says where one of its arrays ends, so an array of more rows
than the section covers ran past that end and one of fewer stopped short of it —
either way the reader takes the values on the wrong side of the boundary for the
keyword line that should be there, which costs every array after it in the
section as well. VTK's own reader stopped on such a file with
Unsupported cell attribute type.Performance
No block is built whole before it is written. A run of cells that share a node
count — every mesh of one element type, and any mixed one whose types happen to
agree — is laid out as rows of an array rather than a cell at a time, and both
spellings share that layout; so do the ragged rows a mixed mesh spells, which
take a row at a time off one shared iterator rather than slicing the run per
cell.
CELL_TYPESis translated by indexing a table built once at import ratherthan by a dictionary hop per cell, and nothing is sized by the codes themselves,
so a code no table names costs a pass rather than an allocation the size of the
code.
Writing 400k triangles:
The bytes written are unchanged across both versions, both encodings, uniform
and mixed meshes, but for one: a
SCALARSblock of two to four components nowputs a tuple to a line where it ran them all onto one, which is the layout every
other multi-component section already had.
Interop guard
The guard that holds the writer to a reader other than this one asks VTK's own,
which is what defines the format, and fails on a message to its error window
rather than only on a grid it could not build.
That reader is quiet about more than it refuses — a block left unclosed and a
SCALARSwider than four both go straight through it — so its silence onlymeans something once the window is known to be live. A file it does reject is
read through the same window first, and the window is process-global, so it is
put back on the way out rather than left pointing into a deleted temp dir.
VTK is never a dependency: it has no buildable sdist, so declaring it would
break
pip install -e ".[dev]"wherever no wheel is published. It is installedby a CI job of its own, which is where these guards run rather than skip.