Skip to content

test(vetting): a golden reference table is const, and read with .at() - #424

Open
darkclad wants to merge 3 commits into
PolusAI:mainfrom
darkclad:main-ref-vals-const
Open

test(vetting): a golden reference table is const, and read with .at()#424
darkclad wants to merge 3 commits into
PolusAI:mainfrom
darkclad:main-ref-vals-const

Conversation

@darkclad

Copy link
Copy Markdown

53 files, all under tests/. No src/nyx change.

Follow-up to #422, which gave every golden table one name, one location and one declaration type.
This adds the property those three could not express: the table is read-only data, and saying so
in the type closes a way for an assertion to pass against a golden that does not exist.

The defect

53 of the 61 reference tables were declared without const, and 54 read sites reached them through
operator[]:

static ref_vals_map<double> firstorder_2d_ibsi_ref_vals { ... };          // not const
ASSERT_TRUE(agrees_gt(total, firstorder_2d_ibsi_ref_vals[feature_name], 100.));

On a key the table does not hold, operator[] default-inserts 0 rather than failing. The
assertion then compares against a golden that was never written down — and agrees_gt derives its
band from the golden:

auto tolerance = ground_truth / frac_tolerance;      // golden 0 -> tolerance 0
bool good = std::abs(fval - ground_truth) <= std::abs(tolerance);

So a golden of 0 passes exactly when the computed value is also 0. That is not an unreachable
corner: "the feature was not computed and returned 0" is one of the specific failures a golden
table exists to catch, and in that combination the test goes green against a reference that isn't
there. The inserted key also persists for the rest of the run, so a later .count() guard on the
same key succeeds too.

Same class the #422 review kept surfacing — an assertion that cannot fail on the thing it claims to
check — reached through the container's API instead of through naming or file layout.

The change

const on all 61 tables, .at() at the 54 read sites. operator[] does not exist on a const
map, so every site needing attention arrived as a compile error, not as a silent change in
behaviour — which is what makes a sweep this wide safe to do mechanically. A missing key now throws
naming the key instead of inventing a zero.

Worth noting for reviewers weighing risk: the first pass keyed the rewrite on table names, and the
compiler caught the two _ref_tols tolerance tables it missed. The second pass is driven off the
declarations, so the set is closed by type rather than by spelling.

Two neighbour oracle files guarded a key they had already indexed. test_2d_neighbor_analytic.h
and test_2d_neighbor_cellprofiler.h wrote
..._ref_vals_by_label.at(label).count(feature_name) > 0, so an absent label threw out of .at()
with no context rather than failing the assertion. They now guard the label first, as
test_2d_neighbor_regression.h already did.

Enforced. check_test_names.py rejects a reference table declared without const, beside the
existing raw-container and _common.h rules. The self-test plants a table whose name, location and
type all conform so that mutability is its only defect, and asserts the rule does not fire on a
const table — a rule that flags everything is as useless as one that flags nothing. SPEC §6.3.1 and
test_ref_vals.h record the rule and why this is a correctness problem rather than a style one.

One cleanup that belonged with it. test_2d_remaining_common.h emptied out when its tables moved
to their assertions, and the sweep left <gtest/gtest.h> and <string> unused plus three comment
blocks describing tables no longer in the file. Same fix test_2d_neighbor_common.h got in #422; this
file had emptied after that landed.

What this did and did not find

No existing assertion was wrong. Every test passes, and nothing threw from .at(), so no test in
the tree was relying on a default-inserted key today. The change closes the hole prospectively and
makes reopening it a build failure; it did not uncover a live bad assertion.

Verification

  • gtest: 732/732 (Windows), 728/728 (Linux ASan) — the four-test delta is the DICOM HU-loader
    mechanics tests, absent from a NOEXTRAS=ON build
  • ASan + UBSan: exit 0, no reports (-fsanitize=address,undefined -fno-sanitize-recover=undefined, RelWithDebInfo, gcc); 113 s vs 22 s uninstrumented, so the
    instrumentation was live
  • pytest tests/python/: 85 passed / 1 skipped (7 pre-existing Arrow failures of a tiff-only build)
  • check_test_names.py --check and check_coverage.py --check clean on both platforms; 61 tables
    inspected, 0 raw, 0 non-const, 0 in a _common.h; vetting self-tests 7/7 with the new case
  • Diff entirely under tests/ — no src/nyx change
  • LF throughout; git diff --shortstat and --ignore-all-space --shortstat agree, so no
    whitespace-only churn

Commits

  1. test(vetting): drop unused includes and stale table comments from the 2D remaining fixture
  2. test(vetting): make every golden reference table const and read it with .at()
  3. test(vetting): reject a golden reference table declared without const

Not in this PR

The constexpr form asked for in #422 review. std::unordered_map with std::string keys
allocates, so the compile-time shape is
constexpr std::array<std::pair<std::string_view, double>, N> with a constexpr accessor. The
payoff usually claimed for it — a mistyped key caught at build time — is not available while keys
arrive as runtime std::string parameters into the shared assert_* helpers, and after this PR it
changes no behaviour. Recorded as optional follow-up rather than dropped.

@darkclad
darkclad force-pushed the main-ref-vals-const branch from 12721ca to e217330 Compare August 14, 2026 14:31
Demian Vladi added 3 commits August 14, 2026 08:24
… 2D remaining fixture

The sweep that moved this header's reference tables out to the files whose
assertions read them left three comment blocks behind describing tables that are
no longer here, and <gtest/gtest.h> and <string> with nothing using them -- the
header asserts nothing, it only builds ROIs, and the five files that include it
bring gtest in themselves.

Same cleanup test_2d_neighbor_common.h got; this file had emptied out after that
fix landed. The ellipse-fixture rationale moves down beside the builder it
describes, since its "the fixture above" reference no longer pointed at anything.
…th .at()

58 of the 69 tables were declared without const, and 55 read sites reached them
through operator[]. On a missing key operator[] default-inserts 0 rather than
failing, so the assertion compares against a golden that does not exist -- and
agrees_gt derives its tolerance from the golden, so a golden of 0 gives a
tolerance of 0 and the assertion passes whenever the computed value is also
exactly 0. That is the case a golden table exists to catch: a feature that
silently produced nothing. The inserted key then persists for the rest of the
run, so a later .count() guard on it succeeds too.

const makes operator[] a compile error and forces .at(), which throws naming the
key it could not find. Every site that needed changing surfaced as a build error
rather than as a silent change in behaviour.

The two neighbour oracle files called .at(label) inside their own presence guard,
so an absent label threw instead of asserting; they now guard the label first, as
the regression file already did.
The const rule is only worth as much as its enforcement, so check_test_names.py
now rejects a reference table declared mutable, alongside the raw-container and
_common.h rules. The self-test plants a table whose name, location and type all
conform, leaving mutability as its only defect, and asserts the rule does not
fire on a const table -- a rule that flags everything is as useless as one that
flags nothing.

SPEC 6.3.1 and test_ref_vals.h record the rule and why operator[] on a reference
table is a correctness problem rather than a style one.
@darkclad
darkclad force-pushed the main-ref-vals-const branch from e217330 to bdf80aa Compare August 14, 2026 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant