chore(vetting): one coverage-scanner library, and a feature report that checks the registry against the tree - #470
chore(vetting): one coverage-scanner library, and a feature report that checks the registry against the tree#470darkclad wants to merge 7 commits into
Conversation
…erage artifact Every other family regenerates its feature -> test map by reading the test sources, so the artifact cannot drift from the tree. 3D GLDZM had none: scan_gldzm_coverage.py is 2D-only, and the 18 3D rows were the only ones in the registry with no scanned counterpart. A gldzm_3d_mirp_vetting_report.md already existed, so the vetting had been done; only the artifact was missing. scan_gldzm3d_coverage.py + gldzm_3d_coverage.csv close that, modelled on the 3D NGLDM scanner -- the other family with no oracle test, for the same reason. ORACLE_SUFFIX is deliberately empty: MIRP is config-matched and reproducible but disagrees with Nyxus on all 16 features it computes, by up to 67.5x, and an independent from-definition recomputation reproduces MIRP to rel 3.2e-16, so the gap is Nyxus'. Nothing here may be promoted on the current implementation. Its --check runs the assertion-identity tier as well as the core checks, which is the tier that was missing when 3GLDZM_ZDM carried a golden of 222 -- a factor of 14.5 off -- with no test function and no TEST() registration, so nothing could fail on it. gldzm_3d_golden_regen.md records the regeneration path and one gap it does not close: this is the only 3D family with no *_dump_regression test, so its pin table is transcribed by hand. That is a C++ change and needs its own pass under the ASan gate. Gates: scan_gldzm3d_coverage.py --check clean, and reports the fault under three negative controls (unregister a case, remove a feature from an assertion line, point a row at a bogus test_name). check_coverage.py --check and check_test_names.py --check both clean.
not_covered.md already recorded that the coverage logic is copied across every per-family scan_* file. The cost was measurable: the twenty had drifted into four spellings of the function regex and two of the comment stripper, and a fix applied to one copy stayed in one copy. audit/scanlib.py is that logic once -- the coverage rule, the scan, the acceptance checks, the artifact rendering and the run loop. Each scan_*_coverage.py becomes the family's declaration on top of it: which files to read, how the family spells its feature names in C++, which checks apply. 4,987 lines across 19 scanners become 1,705 across 20 plus a ~530-line library, and thirteen families are now a docstring and one Family(...) call. What is NOT unified, because the difference is real and belongs where the family is declared: 2D NGTDM, IMQ, 3D GLDM and 3D GLSZM read current_test per test KIND -- a vetted row answers to the oracle files, a regression row to the snapshot one -- which is stricter than the shared per-feature reading and catches a class it cannot. 2D moments resolves coverage through golden tables because nothing in it names a feature on an assertion line. 2D radial declares its mechanics file UNCREDITED, since those pins characterize known defects and crediting them would make the defects acceptance criteria. Each keeps its own collect or checks and says why in its own docstring. THREE BEHAVIOUR CHANGES, all in 2D morphology, all corrections. 1. helper_features bounded a pytest helper's body at the next HELPER rather than the next top-level def, so the last helper in a file swallowed every test function below it and each caller inherited every feature name in the file. Three scanners carried the correct bound and eleven did not. scanlib takes the correct one, which exposes eight current_test cells naming a file that covers nothing for them, corrected here: four chord rows named test_2d_morphology_invariant.py, which reads MAXCHORDS_MAX/MIN and ALLCHORDS_MAX/MIN in an if-guard and asserts only the _ANG features; four PERIMETER rows named test_2d_ooc_invariant.py, whose own docstring says "PERIMETER is deliberately NOT among them: it does NOT agree, and that divergence is pinned as a regression". Each of the eight keeps its other files. 2. A pytest case can assert features that rule cannot see. test_2d_ooc_invariant.py names the six ellipse features in a local tuple and EROSIONS_2_VANISH in the comprehension that builds its compared-column list, then loops that list while asserting, so no name reaches an assertion line -- and under the corrected bound those seven would have read as uncovered, which would be wrong. scanlib gains py_loop_tables, the Python twin of the loop_tables concession the C++ equivalence tests already had, gated the same way: the function must both assert and range-loop, so a plain lookup list is not coverage. Opt-in; 2D morphology is the only family that takes it. 3. morphology_2d_coverage.csv repeated eight features four times each -- 123 lines for 99 features, byte-identical repeats carrying no extra information. That scanner rendered one line per REGISTRY row while the registry has moved to one row per config cell. The artifact is a feature -> test rollup, so scanlib renders one line per feature and the 24 duplicates are gone. Behaviour was held identical everywhere else, gated per scanner on its artifact AND its --check output under a four-mutation battery against the registry -- blank a test_name, force a row to vetted, point current_test at a missing file, corrupt an oracle token. The baseline was captured from the pristine scanners on this base, not carried over from before the rebase. Gates: 20/20 scanners --check clean, check_coverage.py --check clean and --write reproduces coverage_report.md unchanged, check_test_names.py --check 0 violations, pytest tests/python/ 98 passed / 1 skipped / 7 failed -- the 7 are pre-existing HU and signed-int16 failures that reproduce identically on an untouched worktree with the same extension, and no C++ or python test is touched here.
oracle_coverage.csv records what a row CLAIMS -- a status, an oracle, a tolerance, a recipe. The per-family scanners record what the TREE ASSERTS -- which gtest case covers which feature, under which oracle, with which drift guard behind it. Neither could check the other, and the two were only ever compared per family, by hand, when somebody happened to run a scanner. coverage_report.md is a sixteen-row tally of the registry against itself; it cannot see the tree at all. report_features.py joins them into features.csv and features.md, one row per (dim, feature, oracle, config_recipe) -- the key the registry is already unique on, and the one that keeps a feature's config cells apart, which matters because a feature vetted at ibsi=true can diverge by two orders of magnitude at ibsi=false. Columns are grouped claim_* / scan_* so a reader can see which side of the join a value came from, and the derived `verdict` column says whether the two agree. That is what makes this a gate rather than a summary: --check fails on any row whose verdict is not `agree` and is not allowlisted with a written reason. It reads the tree THROUGH THE SCANNERS, importing each audit/scan_*_coverage.py and running its FAMILY declaration, so the report and the per-family --check cannot disagree about what a test covers. That is only possible because they now share a library; without it a tree-wide report would mean a twenty-first copy of the coverage rule, which is precisely how report_feature_tests.py came to credit tests with features they never checked. WHAT IT FOUND. 745 of 923 rows agree and none disagree, but 178 (19%) come back `unscanned`: five family x dim combinations -- 2D firstorder (36 rows), 2D gabor (2), 2D glcm (59), 2D glrlm (32) and 3D firstorder (49) -- have an audit/<family>_<dim>_coverage.csv with no scanner behind it. Those artifacts record what someone believed rather than what the tree asserts, and nothing anywhere could say so. They are declared in KNOWN_UNSCANNED with a reason each, features.md gives the number a section of its own, and --check fails if a sixth family appears undeclared. Writing those five scanners is the next branch; two of them have a working 3D twin to copy. CI gains two steps beside the existing two: the twenty scanners in one loop, and report_features.py --check. The scanners are stdlib-only and need no build, so the cost is seconds. Gates: report_features.py --check clean, and it fails correctly under two negative controls -- a registry row given an oracle no test carries, and a hand-edited artifact. 20/20 scanners --check clean, check_coverage.py --check clean, check_test_names.py --check 0 violations.
vjaganat90
left a comment
There was a problem hiding this comment.
Overall looks good but these need a tweak.
| if key not in cov: | ||
| return "unscanned" | ||
| c = cov[key] | ||
| f, status, oracle = row["feature"], row["status"].strip(), row["oracle"].strip() |
There was a problem hiding this comment.
[Blocker] The verdict is not actually config- or assertion-specific. The report declares (dim, feature, oracle, config_recipe) as its key because configurations can disagree by orders of magnitude, but verdict_of() only consults feature-wide oracle/regression buckets. It never uses config_recipe, claim_test_name, or the row's claimed file to select the corresponding assertion. I reproduced this with the vetted 3GLSZM_GLN row: changing its recipe from glszm3d.pyradiomics_bincount20 to either glszm3d.regression_constant_roi or glszm3d.regression_ut_phantom_nobinning still returns agree. The generated rows likewise receive the same complete list of oracle and regression tests regardless of which configuration they claim. Please make the scanned evidence row-scoped—ideally by resolving a general assertion identifier to its function and configuration—or narrow the report's contract to feature-level coverage. As written, the central config-aware guarantee is not enforced.
There was a problem hiding this comment.
Fixed — the verdict is assertion-specific where the row identifies an assertion, config-specific where the family can say what a config looks like, and the rest now says so per row instead of implying otherwise.
test_nameresolves throughtest_all.ccto the function it runs, and the verdict asks whether that function is the row's evidence. 307 rows carry one; all 307 resolve and agree, so this is a strengthening the tree already satisfied.Family.recipe_readermaps a recipe to the pattern its asserting function's name must match. 3D GLDM had one; 3D GLSZM and 3D NGTDM gain one — the three families whose recipes are distinguishable by function name rather than by file or kind. 151 rows are config-checked, and your reproduction (3GLSZM_GLN's recipe swapped forglszm3d.regression_constant_roi) now returnsrecipe-mismatch.- A new
verdict_scopecolumn readsrow+config(151) /row(156) /feature(438) /none(178). The 438 carry notest_nameat all, so nothing identifies the assertion they describe — the honest reading is feature-level, and the column says that rather than letting the key imply more.PR/todo.mditem 58 tracks closing them per family. scan_row_testsshows the functions the row's owntest_nameresolved to.
Controls are in audit/scanlib_selftest.py (runs in CI): swapped recipe, undeclared recipe, test_name pointing at another feature's case, test_name naming no case, and a blanked test_name falling back to feature.
| if fam.loop_tables and re.search(r"for\s*\([^)]*:\s*\w+\s*\)", block): | ||
| for m in re.finditer(r"=\s*\{(.*?)\};", block, re.S): | ||
| hits.setdefault(fn, set()).update(feat_re.findall(m.group(1))) | ||
| for name, feats in helpers.items(): |
There was a problem hiding this comment.
[Blocker] Helper propagation still converts unasserted values into coverage. Every feature read by a helper is credited to every test that calls that helper, even when the caller discards part of the result. There is a current example in test_2d_morphology_fraclac.py: _fd() returns both FRACT_DIM_BOXCOUNT and FRACT_DIM_PERIMETER; the box-count test does bc, _ = _fd(...), while the perimeter tests do _, pf = _fd(...). Nevertheless, the generated artifact credits all three tests with both features. That contradicts the stated rule that coverage requires an assertion. Please make helper attribution aware of which returned value reaches the assertion, or provide an explicit family-level mapping for multi-value helpers. A negative test for “helper returns two features, caller asserts only one” would pin the intended behavior.
There was a problem hiding this comment.
Fixed — helper attribution is now per returned position, and a caller is credited only with the positions it both binds and asserts on.
helper_positions() maps each element of a helper's tuple return to the features that reach it, following the local the column lookup named (bc = [c for c in row.index if c.endswith("FRACT_DIM_BOXCOUNT")][0], then return float(row[bc]), float(row[pf])). helper_credit() reads the caller's unpacking against that: _ is dropped, and so is a bound name that never reaches an assertion line. A caller that does not unpack keeps the whole helper, as does any feature the return positions do not account for.
Your example is the measured result — morphology_2d_coverage.csv changes in exactly two cells:
FRACT_DIM_BOXCOUNTlosestest_2d_morphology_perimeter_disk_analyticand..._koch_snowflake_analyticFRACT_DIM_PERIMETERlosestest_2d_morphology_boxcount_known_dimension_analytic
Both keep the gtest case test_2d_morphology_fractal_dimension_blob512_fraclac, which asserts both on its own assertion lines and does not go through _fd.
The negative test you asked for is in audit/scanlib_selftest.py: a helper returning two features with one caller per half, a caller that does not unpack (keeps both), a single-value helper (unchanged), and a caller that binds the second value but never asserts on it (still not credited).
| equivalence tests, and it is gated the same way: the function has to both assert and range-loop, | ||
| so a plain lookup list is not mistaken for coverage. | ||
| """ | ||
| if not PY_LOOP_NAME.search(block): |
There was a problem hiding this comment.
[Blocker] py_loop_tables credits every local literal when any loop exists. This check only establishes that the function contains some for … in <name> loop; the code then extracts features from every local list or tuple literal without checking whether that literal feeds the loop or its assertion. For example, a function containing unused = ("PERIMETER",), checked = ("AREA",), and a loop over checked reports both features as covered—I reproduced that directly through py_literal_features(). This reintroduces the false-positive coverage this refactor is meant to remove. Please associate each assignment's left-hand name with the collection that is actually iterated—following the current ellipse → cols flow where needed—and add an unused-literal-beside-a-real-loop negative control.
There was a problem hiding this comment.
Fixed — literals are resolved by name against the loop, so a literal the loop never reaches is not credited.
PY_ASSIGN_LITERAL now captures the assigned name and PY_LOOP_NAME the iterated one; only the iterated literal is credited, plus any literal a reached literal names. That second hop is the ellipse → cols flow you pointed at: cols = [c for c in df_ram.columns if ... or c in ellipse], then for c in cols:.
Your exact case is a control in audit/scanlib_selftest.py — unused = ("PERIMETER",) beside checked = ("AREA",) and a loop over checked credits AREA only. The seven features the concession exists for in test_2d_ooc_invariant.py are credited exactly as before, so no artifact moves.
Your follow-up (loop not tied to an assertion) is answered on the round-2 comment below and is fixed too.
Six findings from the review of this PR. `no_oracle_claimed` was silently dropped for 2D neighbor and 2D zernike. Three pre-refactor scanners ran the reverse of `oracle_mismatch` -- an oracle-suffixed test asserts a feature the registry says has no oracle -- and only radial carried it into the shared library. Blanking the nine oracle cells of those two families now reports 8 and 1 problems again, as it did before the refactor. `pointers()` resolved a 3D row's config matrix by falling back to the 2D file when no `<family>3d.md` exists. 94 rows pointed at a matrix for a different calculator: 3D gldzm and 3D morphology at files that never mention 3D. A 3D row now links a 3D matrix or nothing, which is what every other missing pointer in the report already renders. The `vetting_report` pointer was keyed on the row's CLAIMED oracle, so a family whose oracle disagrees with Nyxus -- and which therefore deliberately claims none -- could never link the report that recorded the disagreement. 3D gldzm and 3D ngldm now name their MIRP reports. The claimed oracle still wins, then the oracles the tree asserts, then the family's own report where it has exactly one; 235 rows gain a pointer and no existing pointer changes. Also: the oracle matrix is keyed on (dim, family), so 2D morphology's MATLAB vetting no longer shares a cell with the 3D MIRP one; the CI loop runs all twenty scanners before failing rather than stopping at the first, which is the right signal for a shared library; the count of missing scanners is read from KNOWN_UNSCANNED rather than spelled out; 3D GLDM's write line reports its feature count again; and the README says which of the two `features.csv` it means.
Three blockers from review, each reproduced before the fix and controlled after it.
py_loop_tables credited every local literal whenever the function looped at all: the
gate only established that some `for x in name:` existed, then read features out of
every list or tuple literal in the block. A case holding `unused = ("PERIMETER",)`
beside a real loop over something else reported PERIMETER as covered. Literals are
resolved by name now -- the one the loop iterates, plus whatever a reached literal
itself names, which is the `cols` built from `ellipse` shape the concession exists
for. The seven features it credits in test_2d_ooc_invariant.py are unchanged.
A multi-value pytest helper credited each caller with the values it discards. _fd()
in test_2d_morphology_fraclac.py returns both fractal dimensions; the box-count case
takes `bc, _ = _fd(...)` and the two perimeter cases take `_, pf = _fd(...)`, and all
three were credited with both. Helper attribution now maps each returned position to
the features reaching it and credits only the positions the caller binds and asserts
on; a caller that does not unpack keeps the whole helper. Two cells of
morphology_2d_coverage.csv change and nothing else does.
The report's verdict was feature-scoped while its key was config-scoped: verdict_of()
never read config_recipe or test_name, so a vetted row's recipe could be swapped for
another of its family's and stay `agree`. The evidence is row-scoped where the row
names its assertion -- test_name resolves through test_all.cc to the function it runs,
and the verdict asks whether that function asserts this feature at this row's kind
(307 rows, all resolving and agreeing). The configuration is checked where the family
declares a recipe_reader: 3D gldm had one, 3D glszm and 3D ngtdm gain one, 151 rows.
A verdict_scope column says which of row+config, row, feature or none each row got,
so a weak agreement cannot read as a strong one.
Two smaller ones found while fixing those. case_to_file read a TEST() body to the
first newline-then-`}`, which runs past the 182 one-line registrations into the next
multi-line case; only multi-line families read it today, so nothing was mis-resolved,
but the row-scoped verdict depends on it. Family.fn_prefix defaulted to
test_2d_<family> for any dim that was not 3D, so IMQ's prefix matched nothing.
audit/scanlib_selftest.py puts eighteen negative controls on fixtures where the answer
is known, and runs in CI beside the scanners: a literal beside a real loop, a literal
reached through another, a discarded return value, a bound value that reaches no
assertion, a one-line registration, and five faults injected into a real registry row.
A per-family --check cannot catch any of these -- it compares an artifact to the tree
through the same rule that produced it.
20/20 scanners --check clean, report_features.py --check clean at 923 rows,
check_coverage.py --check clean with coverage_report.md reproducing unchanged,
check_test_names.py 0 violations. No src/ and no tests/python change.
…cumulator The scanner step failed on both pushes that carried the shell accumulator the round-1 commit introduced -- `rc=0`, `|| rc=1`, `exit $rc`, written so that a change breaking the shared library would report all twenty families rather than stopping at the first. Both failures logged twenty `clean` lines, nothing on stderr, and no scanner exiting non-zero; the run before it, with the same scanners and the earlier `|| exit 1` form, was green. The construct does not reproduce. The byte-identical script under `/usr/bin/bash -l` on Ubuntu 24.04 against this tree exits 0, and each of the twenty scanners exits 0 individually there. The runner-side mechanism is therefore not named here; what is removed is the reliance on it. audit/run_scanners.py moves the accumulation into the process that already knows the answer: the same twenty declarations through the same scanlib.run, with an ordinary Python exit status -- the shape every other check step in this workflow already uses. It keeps what the loop was written for and adds two things the loop did not have: it names the families that failed, and a family that raises is counted rather than allowed to abort the other nineteen. It is also one command a developer can run locally, which the loop was not. Verified both ways: 20 of 20 clean and exit 0 on the tree as it stands; with one registry row's current_test pointed at a file that covers nothing, exit 1, `19 of 20 families clean`, `FAILED: scan_zernike_coverage.py`, and the other nineteen still reported. All five vetting gates re-run on Linux: registry clean, 0 naming violations, selftest clean, 20/20 scanners clean, report clean at 923 rows.
| return "test-name-unresolved", scope | ||
| # the row's own assertion, at the row's own kind: a vetted row answers to an oracle-suffixed | ||
| # function, and a feature-wide `asserted` set does not say that THIS case is one of them | ||
| covering = c.asserted.get(f, set()) if status == "vetted" else ( |
There was a problem hiding this comment.
[Blocker] The row check still accepts the wrong evidence kind and oracle. For every non-vetted row, covering unions regression, invariant/other, and oracle functions, so a regression claim can be satisfied by an oracle case. I reproduced this by changing the NGTDM_COARSENESS regression row's test_name to TEST_NYXUS.TEST_2D_NGTDM_COARSENESS_IBSI; it still returns ('agree', 'row'). The vetted analogue also passes: the same feature's oracle=mirp row still agrees when test_name names only the IBSI case, because the oracle check above is feature-wide and is not tied to the resolved function. Once a row supplies test_name, please require its resolved evidence to match the exact claim—claimed oracle for vetted, regression bucket for regression, and the corresponding other bucket for invariant—and add both mutations as negative controls.
There was a problem hiding this comment.
Fixed — a row's evidence is now read at its own kind and its own oracle, and the two ways of missing are told apart.
row_covering() returns the row's own bucket rather than a union: the oracle set filtered to the tool the row claims for vetted, the regression set for regression, the invariant set otherwise. Both of your mutations fail now:
- 2D
NGTDM_COARSENESSregression row →TEST_2D_NGTDM_COARSENESS_IBSIgivesrow-test-wrong-kind - the same feature's
oracle=mirprow naming only the IBSI case givesrow-test-wrong-oracle(naming the MIRP case still agrees)
Filtering by the claimed oracle needs a function → oracle map, which is scanlib.oracle_token: the SPEC 2 name suffix, plus a Family.fn_oracle exception for a function whose suffix names no oracle. I measured that rather than assuming it — test_3d_gldm_compatibility (3D GLDM's PyRadiomics case through the Python API) is the only asserted function in the tree in that position, and the map it already used privately is declared on the Family now so the report reads the same one.
Both mutations are pinned in audit/scanlib_selftest.py, each beside the unmutated row. No row in the registry fails the stricter rule: 745 still agree and no artifact changes.
| recipe = row["config_recipe"].strip() | ||
| if recipe not in readers: | ||
| return "recipe-unreadable", "row+config" | ||
| if not any(readers[recipe].match(fn) for fn in fns): |
There was a problem hiding this comment.
[Blocker] Feature and recipe can be proven by different functions. The feature check and recipe check are separate existential tests over fns, so two individually wrong cases can combine into row+config. On the 3GLSZM_GLN pyradiomics_bincount20 row, I set test_name to TEST_NYXUS.TEST_3D_GLSZM_IBSI_GAPPED_PYRADIOMICS;TEST_NYXUS.TEST_3D_GLSZM_SAE_PYRADIOMICS: the first asserts GLN at the wrong recipe, the second matches the requested recipe but asserts SAE, yet the verdict is still ('agree', 'row+config'). Please require one resolved function to be both in the row's exact covering set and matched by the recipe reader, and pin this split-evidence case in the self-test.
There was a problem hiding this comment.
Fixed — one resolved function has to answer the whole row.
The two tests are no longer independent: the recipe reader now ranges only over hits, the functions already established as this row's evidence by row_covering(), so a case that matches the recipe but asserts a different feature can no longer supply the half the other case is missing.
Your split-evidence case is the control, in audit/scanlib_selftest.py: 3GLSZM_GLN's pyradiomics_bincount20 row with test_name set to TEST_3D_GLSZM_IBSI_GAPPED_PYRADIOMICS;TEST_3D_GLSZM_SAE_PYRADIOMICS now returns ('recipe-mismatch', 'row+config') instead of agree.
| literals = {} | ||
| for m in PY_ASSIGN_LITERAL.finditer(block): | ||
| literals.setdefault(m.group(1), []).append(_bracketed(block, m.end() - 1)) | ||
| reached = {n for n in PY_LOOP_NAME.findall(block) if n in literals} |
There was a problem hiding this comment.
[Blocker] py_loop_tables now ties a literal to a loop, but still does not tie that loop to an assertion. scan() only establishes that the function contains some assertion somewhere. For example, checked = ("AREA",) followed by for c in checked: print(c) and then an unrelated assert 1 == 1 still reports AREA as covered. That conflicts with the documented rule that the function loops the table while asserting and still lets a lookup/logging loop become coverage. Please require the relevant loop body to contain the assertion (and ideally use its iterator), then add a loop-plus-unrelated-assert negative control.
There was a problem hiding this comment.
Fixed — the loop body has to do the comparing, not merely exist.
A for x in NAME: credits NAME only if its body either asserts, or accumulates into a name the function later asserts on. Your example — for c in checked: print(c) beside an unrelated assert 1 == 1 — credits nothing now.
One note on why it is not "the assertion must be inside the loop body": the case this concession exists for does not do that. test_2d_ooc_invariant.py collects into bad inside the loop and asserts not bad outside it, which is the ordinary accumulate-then-assert shape. Requiring the assertion inside the body would have dropped the seven features the rule was written to credit, so the rule follows the value instead: the body has to feed something the function asserts.
Both directions are controls in audit/scanlib_selftest.py — the logging loop credits nothing, and the accumulate-then-assert loop credits exactly the two features its list names — so the rule cannot be tightened into dropping the real case or loosened back into accepting the logging one.
Three blockers from the third review, each reproduced before the fix. All three
are the shape the round-2 commit itself closed twice: a rule that answers a
weaker question than the one it is written to answer, and passes.
A row's evidence was checked for the right feature but not for the right kind or
the right tool. For a non-vetted row the covering set unioned the regression,
invariant and oracle buckets, so a regression row could be satisfied by an oracle
case -- the drift guard reads as present when only the oracle test exists.
Pointing 2D NGTDM_COARSENESS's regression row at TEST_2D_NGTDM_COARSENESS_IBSI
returned `agree`. The vetted half was the same mistake through another door: the
oracle check above it is feature-wide, so the same feature's oracle=mirp row
agreed while naming only the IBSI case. row_covering() now returns the row's own
bucket -- the oracle set filtered to the tool the row claims for vetted, the
regression set for regression, the invariant set otherwise -- and the two
failures are told apart as row-test-wrong-kind and row-test-wrong-oracle.
Reading the claimed oracle off a function needs scanlib.oracle_token: the name
suffix per SPEC 2, plus a family-declared exception for a function whose suffix
names no oracle. 3D GLDM asserts against PyRadiomics through the Python API in
test_3d_gldm_compatibility, and it is the only such function in the tree, which
was measured rather than assumed. The map already existed inside that family; it
is declared on the Family now, so the report reads the same one.
The feature and the recipe could be proven by different functions. They were two
existential tests over the same list, so a row naming two individually wrong
cases passed: on 3GLSZM_GLN's pyradiomics_bincount20 row,
TEST_3D_GLSZM_IBSI_GAPPED_PYRADIOMICS asserts GLN at the wrong recipe and
TEST_3D_GLSZM_SAE_PYRADIOMICS runs at the right recipe asserting SAE, and the
verdict was `agree`, `row+config`. The recipe test now ranges only over the
functions already established as this row's evidence.
py_loop_tables tied a literal to a loop but not the loop to an assertion. scan()
establishes only that the function asserts somewhere, so `checked = ("AREA",)`
followed by `for c in checked: print(c)` and an unrelated `assert 1 == 1`
credited AREA. The loop body must now either assert or accumulate into a name the
function later asserts on, which is the shape the tree uses --
test_2d_ooc_invariant.py collects into `bad` inside the loop and asserts not bad
outside it. Requiring the assertion inside the body would have dropped the seven
features the concession exists for.
Nothing in the report or the twenty artifacts moves: 745 rows still agree, the
scope split is unchanged at 151 row+config / 156 row / 438 feature / 178 none,
and no audit/*_coverage.csv changes. The self-test grows from 18 controls to 26 --
the split-evidence case, the wrong-kind and wrong-oracle cases, the logging loop,
and the accumulate-then-assert positive that guards against over-tightening it.
All five vetting gates re-run on Windows and on Linux: registry clean, 0 naming
violations, 26/26 controls, 20/20 scanners, report clean at 923 rows.
30 files, 29 under
tests/vetting/and one CI workflow. Nosrc/nyxchange —git rev-parse HEAD:srcequalsgit rev-parse upstream/main:src. Net −1,133 lines.Four commits, all
chore(vetting), so nothing here reaches the CHANGELOG and nothing moves theversion whether this is squashed or merge-committed. That is deliberate:
release-please-config.jsonsends
featto Features and bumps the patch pre-1.0, andrefactorto Code Refactoring, and none ofthis is observable to anyone using Nyxus.
The problem
oracle_coverage.csvrecords what a row claims — a status, an oracle, a tolerance, a recipe.The per-family scanners under
audit/record what the tree asserts — which gtest case coverswhich feature, under which oracle, with which drift guard behind it.
Neither could check the other.
coverage_report.mdis a sixteen-row tally of the registry againstitself; it cannot see a test at all. The two sides were only ever compared per family, by hand, when
somebody happened to run a scanner — and five families had no scanner to run.
That is the gap this closes, and it needed the three steps in order.
1. The one family with no coverage artifact
3D GLDZM was the only family in the registry with no coverage artifact at all:
scan_gldzm_coverage.pyis 2D-only, so its 18 rows had nothing to be checked against, scanned or hand-written. Five other
family x dim combinations do have an artifact, but nothing re-derives it from the tree -- the report
names all five under Families with no scanner, and writing those scanners is separate work.
A
gldzm_3d_mirp_vetting_report.mdalready existed, so the vetting had been done; only the artifactthat re-derives it from the tree was missing.
scan_gldzm3d_coverage.py+gldzm_3d_coverage.csvclose it, modelled on the 3D NGLDM scanner —the other family with no oracle test, for the same reason.
ORACLE_SUFFIXis deliberately empty:MIRP is config-matched and reproducible, but disagrees with Nyxus on all 16 features it computes by
up to 67.5×, and an independent from-definition recomputation (26-connected zones, city-block
distance transform) reproduces MIRP to rel 3.2e-16. The definition is reachable and Nyxus is not
computing it, so nothing here may be promoted on the current implementation.
Its
--checkruns the assertion-identity tier, which is the tier that was missing when3GLDZM_ZDMcarried a golden of
222— a factor of 14.5 off — with no test function and noTEST()registration, so nothing could fail on it.
gldzm_3d_golden_regen.mdrecords the regeneration path and one gap it does not close: this is theonly 3D family with no
*_dump_regressiontest, so its pin table is transcribed by hand. That is aC++ change and needs the ASan gate; it is not in this PR.
2. One library instead of twenty copies
not_covered.mdalready recorded that the coverage logic is copied across everyscan_*file. Thecost was measurable: the twenty had drifted into four spellings of the function regex and two of the
comment stripper, and a fix applied to one copy stayed in one copy.
audit/scanlib.pyholds it once — the coverage rule, the scan, the acceptance checks, the artifactrendering and the run loop. 4,993 lines across 19 scanners become 1,715 across 20, plus a 550-line
library. Thirteen families are now a docstring and one
Family(...)call, the shortest 33 lines.What is deliberately not unified, because the difference is real and belongs where the family is
declared:
current_testper test kind — a vetted row answers to the oracle files, a regression row to the snapshot one. Stricter than the shared per-feature reading, and catches a class it cannottest_2d_radial_mechanics.hUNCREDITED — its pins characterize known defects, so crediting them would make the defects acceptance criteriaEach keeps its own
collector checks and says why in its own docstring.Three behaviour changes, all in 2D morphology, all corrections
a.
helper_featuresover-credited coverage in 11 of 14 scanners. A module-level pytest helper'sbody was bounded at the next helper rather than the next top-level
def, so the last helper in afile swallowed every test function below it and each caller inherited every feature name in the
file. Three scanners carried the fix; eleven did not. scanlib takes the correct bound, which exposes
eight
current_testcells naming a file that covers nothing for them:MAXCHORDS_MAX/MIN,ALLCHORDS_MAX/MIN) namedtest_2d_morphology_invariant.py,which reads those four in an
ifguard and asserts only the_ANGfeatures;PERIMETERrows namedtest_2d_ooc_invariant.py— whose own docstring says so:"PERIMETER is deliberately NOT among them: it does NOT agree, and that divergence is pinned as a
regression." The rows and the file contradicted each other outright and nothing could see it.
Each of the eight keeps its other files; none is left empty.
b. A pytest case can assert features that rule cannot see.
test_2d_ooc_invariant.pynames thesix ellipse features in a local tuple and
EROSIONS_2_VANISHin the comprehension that builds itscompared-column list, then loops that list while asserting — so no name reaches an assertion line.
Under the corrected bound those seven would have read as uncovered, which would have been wrong:
the case really does compare them. scanlib gains
py_loop_tables, the Python twin of theloop_tablesconcession the C++ equivalence tests already had, gated the same way — the functionmust both assert and range-loop, so a plain lookup list is not coverage. Opt-in; 2D morphology is the
only family that takes it.
c.
morphology_2d_coverage.csvrepeated eight features four times each — 123 lines for 99features, byte-identical repeats carrying no extra information. That scanner rendered one line per
registry row, and the registry has moved to one row per config cell. The artifact is a
feature→test rollup, so scanlib renders one line per feature and the 24 duplicates are gone. Any
family whose registry grows config-cell rows would have hit this.
3. The report, and the CI gate
report_features.pygeneratesfeatures.csv(923 rows) andfeatures.md, one row per(dim, feature, oracle, config_recipe)— the key the registry is already unique on, and the onethat keeps a feature's config cells apart, which matters because a feature vetted at
ibsi=truecandiverge by two orders of magnitude at
ibsi=false.--checkenforces that uniqueness; it was truebut unchecked, which is the shape of #443's C3.
Columns are grouped
claim_*/scan_*so a reader can see which side of the join a value camefrom, and the derived
verdictsays whether they agree. That is what makes this a gate ratherthan a summary.
It reads the tree through the scanners themselves, importing each
scan_*_coverage.pyandrunning its
FAMILYthrough scanlib, so the report and the per-family--checkcannot disagreeabout what a test covers. That is only possible because step 2 gave them a shared library; without
it, a tree-wide report means a twenty-first copy of the coverage rule — which is exactly how
report_feature_tests.pycame to credit tests with features they never checked.What it found
745 of 923 rows agree. None disagree. 178 (19%) come back
unscanned:These five have an
audit/<family>_<dim>_coverage.csvwith no scanner behind it. Those artifactsrecord what someone believed rather than what the tree asserts, and no check anywhere could say so —
including all of first-order and all of 2D GLCM, two of the most-claimed families. They are declared
in
KNOWN_UNSCANNEDwith a reason each,features.mdgives the number a section of its own, and--checkfails if a sixth family appears undeclared. Writing those five scanners is the next branch;2D GLCM and 2D GLRLM each have a working 3D twin to copy.
CI gains two steps beside the existing two: the twenty scanners in one loop, and
report_features.py --check. Both are stdlib-only and need no build.4. What review caught
A fourth commit, after review of the three above. Two of the four are the class of defect this PR
exists to prevent — a check that goes quiet, and a generated artifact that says something untrue —
so they are worth naming rather than folding into the others.
A dropped acceptance check, in two families. Three pre-refactor scanners ran
no_oracle_claimed, the reverse oforacle_mismatch: an oracle-suffixed test asserts a featurewhile the row claims no oracle at all. Moving to declarations carried it forward for 2D radial and
silently dropped it for 2D neighbor and 2D zernike — the other two families holding rows that
claim no oracle, which is the only place it can fire. Restored in both declarations. The
regression is reproducible: blank the
oraclecell on those nine rows and the pre-refactorscanners report 8 and 1 problems where the post-refactor ones reported
clean; they now report 8and 1 again. Nothing else covered it —
report_features.py's verdict for avettedrow with anempty oracle cell is
agree, because the empty cell short-circuits the mismatch branch.scanlib.pyhad also recorded the loss as if it were the inherited state ("Only 2D radial runs ittoday"); it now names all three.
94 rows pointed at the wrong dimension's config matrix.
pointers()fell back frommatrix/<family>3d.mdtomatrix/<family>.mdfor a 3D family, and three 3D families have no 3Dmatrix — so 3D GLCM (59 rows), 3D GLDZM (18) and 3D morphology (17) each linked a file documenting
the 2D calculator,
matrix/morphology.mdbeing titled "2D morphology config matrix" outright.The fallback is gone; an empty cell is the honest reading of no 3D matrix exists yet, and it is
what every other missing pointer already renders.
235 rows could not reach their own vetting report. The
vetting_reportpointer was keyed on therow's claimed oracle, so a family whose oracle disagrees with Nyxus — which claims none on purpose
— could never link the report that recorded the disagreement. 3D GLDZM was the clearest case: this
PR adds its scanner, whose
ORACLE_SUFFIX={}is justified entirely bygldzm_3d_mirp_vetting_report.md, and all 18 of its rows rendered an empty cell. The pointer nowtakes the claimed oracle first, then the oracles the tree asserts against, then — only when neither
side names one and the family x dim has exactly one report — that report. Rows with a
vetting_reportgo from 637 to 872.And four smaller ones. The CI scanner loop exited on the first failing family, so a change
breaking the shared library reported one family instead of twenty; it now runs all twenty and fails
once. The family x oracle matrix in
features.mdkeyed on family alone, merging 2D and 3D for thenine family names that exist in both — 2D morphology's
matlab 33and 3D morphology'smirp 8satin one row; it is keyed by dim now, and every column total is unchanged, which is the invariant that
says the old table mislabelled rather than miscounted.
scan_gldm3d's write line had lost itsfeature count. The README described
audit/features.csv, a git-ignored byproduct of the retiredpipeline, two sections below the table linking the new tracked
features.csv; the two share abasename and nothing else, and the README now says so.
No
audit/*_coverage.csvchanged, and outside the three pointer columns nofeatures.csvvaluechanged — row identity and order included.
New artifacts
tests/vetting/audit/scanlib.pytests/vetting/audit/scan_gldzm3d_coverage.py,gldzm_3d_coverage.csvtests/vetting/audit/gldzm_3d_golden_regen.mdtests/vetting/report_features.py--checktests/vetting/features.csv,features.mdHow it was verified
Not "the tests pass" — the scanners' behaviour was pinned and compared.
--checkoutput were recorded under a four-mutation battery applied to the registry: blank a
test_name,force a row to
vetted, pointcurrent_testat a missing file, corrupt an oracle token. Byte-identical CSVs alone would not catch a weakened acceptance check. After the rebase onto
5aca80b9the baseline was re-captured from the pristine upstream scanners on the new base rather than
carried over. All twenty match except 2D morphology's three intended differences above.
case is unregistered, when a feature is dropped from an assertion line, and when a row names a
bogus
test_name.py_loop_tablesis controlled in both directions — emptying the ellipse tupledrops exactly those six features, and a list the function never range-loops is not credited. The
report's key gate fires on a planted duplicate row and its unscanned gate on a deleted scanner.
oraclecell on the nine 2D neighbor and 2D zernike rows makes the restored check report 8 and 1 problems
— the counts the pre-refactor scanners produce on the same injection, and the counts the
post-refactor scanners reported as
cleanbefore the fix.features.csvrow by row. The only cellsthat went from set to empty are the 94 wrong-dimension matrix links; 235 gained a
vetting_report; no non-pointer column changed and row identity and order are unchanged. Each ofthe six family x dim groups the single-report fallback fires on was checked against the report it
links — including the two whose registry spelling differs from the report's
(
gldzm_3d_mirp_vetting_report.mdtabulates all 16 asLDHGLE,LDE, … without the3GLDZM_prefix), which an exact-name search reads as a miss.
--checkclean;check_coverage.py --checkclean and--writereproducescoverage_report.mdunchanged;check_test_names.py --check0 violations;report_features.py --checkclean (923 rows).pytest tests/python/— 98 passed, 1 skipped, 7 failed. The 7 are pre-existing HU andsigned-int16 failures that reproduce identically on an untouched worktree with the same extension;
no C++ and no python test is touched by this branch.
src/is byte-identical toupstream/main.Left open, deliberately
scan_intensity_histogram_coverage.pycounts a bare readout as coverage (fvals[(int)...]lines as well as assertion lines). Every other scanner refuses to, for the reason the shared
docstring gives. Preserved so this refactor changes no behaviour, and isolated in that family's
collect_overridewhere it is visible. Removing it will drop coverage the family currently claims,so the affected rows need re-examining one by one — its own pass.