Skip to content

fix(417)!: use one threshold tolerance at every classification gate - #421

Merged
sromoam merged 6 commits into
awslabs:devfrom
Thanatos9404:fix/417-tolerant-threshold-gates
Oct 29, 2026
Merged

sromoam merged 6 commits into
awslabs:devfrom
Thanatos9404:fix/417-tolerant-threshold-gates

Conversation

@Thanatos9404

@Thanatos9404 Thanatos9404 commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor

Pull Request

Issue Reference

Fixes #417

Description of Changes

Every classification gate now uses ThresholdHelper.is_above_threshold, the test the primitive list element gate already used. A score up to 1e-10 under its threshold now gets the same verdict wherever it is classified.

Changes Made

Line numbers are at 003b5f1.

  • field_comparator.py:68 and :134: the primitive field and nested model verdicts. This is also the clip, so a score inside the tolerance keeps its value instead of becoming 0.0.
  • structured_list_comparator.py:136 and :247: the List[StructuredModel] pair count and the recursion gate. They have to move together, or a pair would count as a TP and still not be descended into.
  • comparison_helper_base.py:120: the list pair verdict behind non_matches and field_comparisons.
  • field_comparison_collector.py:140 and field_comparison_helper.py:136: the match flag on field_comparisons rows, top level and inside list items.
  • confusion_matrix_calculator.py:98: the list item sub-field classifier (Remove the remaining dead confusion-matrix paths left after #390 #395 item 5).
  • auto/facade.py:80: EvalResult.matched, which reads the same match_threshold as list pairing.
  • metrics_helper.py:145: the binary metrics evaluator_format=True falls back to without a confusion matrix.
  • BaseComparator.binary_compare, ComparisonInfo.compare and ComparableFieldConfig.compare (the clip in comparison_info.py), added during review so they agree with the evaluator gates. EvalResult.explain() now reports a clip with is_below_threshold, the same test.
  • algorithms/hungarian.py:325: HungarianMatcher.calculate_metrics tp. The evaluator only reads matched_pairs from it, but its docstring and algorithms/README.md described the exact test as a trap, so I fixed it rather than leave a documented exception. The helper is imported inside the method, since the evaluator package imports hungarian.py while it initializes.
  • Docs: one sentence each on the threshold boundary in classification-logic.md and threshold-gated-evaluation.md; the trap removed from the HungarianMatcher docstring and algorithms/README.md.
  • CHANGELOG.md: an entry under Changed, marked Breaking like the A leading None in List[Optional[Model]] skips the class gate and scores a Cat as a Dog #386 and StructuredModel fields typed date, datetime, time, Decimal, UUID or Enum score 0.0 against an identical value #383 entries under [Unreleased], since counts move at the margin.
  • New tests/structured_object_evaluator/test_threshold_tolerance.py.

Why These Changes Are Needed

The same score got two verdicts depending on where it was classified. Measured on dev at 003b5f1, with 0.7 + 0.1 (0.7999999999999999) against 0.8:

Where dev this branch
List[str] element TP TP
Top-level field FD, score clipped to 0.0, field_comparisons reason below threshold (0.800 < 0.8) TP, score 0.7999999999999999
Nested model field FD, score 0.0 TP, score 0.7999999999999999
List[StructuredModel] pair (the Laptop pair in threshold-gated-evaluation.md) FD, not descended into, one products[0] non-match TP, descended into: product_id TP, name FD (0.4 against 0.7), price TP
Sub-field of a matched list item FD, items[0].name non-match TP
EvalResult.matched at overall_score 0.7999999999999999 False True
HungarianMatcher.calculate_metrics tp 0, fd 1 tp 1, fd 0

A score further under the threshold is unchanged: 0.8 - 1e-9 and 0.79 are FDs before and after.

Left as they are, on purpose:

Related open PRs: #419's caveats on the threshold boundary (the Laptop pair as an FD, "the object and field gates do not" have the tolerance) become untrue with this change, and both PRs edit the same two doc lines. #406 moves list item sub-fields to the field dispatch, which makes the confusion_matrix_calculator.py change here dead; I'll drop it on rebase if #406 lands first. I'll rebase on whichever lands first.

Testing

  • Unit tests added/updated
  • Integration tests added/updated
  • Manual testing completed
  • All existing tests pass (apart from 6 tests that need the POSIX-only signal.SIGALRM; details below)

test_threshold_tolerance.py checks each gate at four scores: one rounding error under the threshold, exactly at it, 1e-9 under it (outside the tolerance) and 0.79. For each gate it checks the counts and, where the gate feeds them, the clipped score, non_matches and field_comparisons. One test checks that a list item sub-field, a top-level field and a List[str] element give the same counts for the same score, and one uses the documented Laptop pair as is. 50 tests: on dev, 13 fail (every rounding error case, plus the Laptop pair) and the 37 controls pass; on this branch all pass.

Windows, Python 3.12.5, uv sync --frozen --extra llm --no-group docs. inspect.getfile(stickler) pointed at this checkout's src/stickler/__init__.py.

uv run pytest -n auto -q passed failed skipped xfailed
dev 003b5f1 2917 6 10 1
this branch 2967 6 10 1

The 6 failures are the same on both trees: 5 tests in TestRealisticDeepNesting and test_integration in test_realistic_deep_nesting.py, all AttributeError: module 'signal' has no attribute 'SIGALRM'. Linux CI runs them.

uv run ruff check passes on the changed Python files. ruff format --check is clean for hungarian.py, facade.py, confusion_matrix_calculator.py, metrics_helper.py and the new test file; the other five were already off ruff format on dev, and none of the drifting lines are ones this PR touches.

Reviewers

@sromoam

Checklist

  • Code follows the project's coding standards
  • Self-review of code completed
  • Documentation updated (if applicable)
  • CHANGELOG.md entry added under [Unreleased], or not needed because
    nothing user-visible changed (see RELEASING.md)
  • Tests added/updated for new functionality
  • All CI checks are passing
  • Ready for review

Additional Notes

  • The tolerance itself is unchanged (1e-10, as ThresholdHelper already had it). I did not add a constant or a new helper; every gate calls the existing method.
  • The issue has the 1.1.0 milestone. If you'd rather this not reach dev before 1.0.1 is cut, I'm fine with it waiting.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

A primitive list element was classified with
ThresholdHelper.is_above_threshold, which accepts a score up to 1e-10
under the threshold. The field and nested-model gates in
FieldComparator, the List[StructuredModel] pair count and recursion
gate, the non_matches and field_comparisons collectors, the list item
sub-field classifier, EvalResult.matched,
HungarianMatcher.calculate_metrics and the evaluator_format fallback
metrics all used a bare >=. A score of 0.7 + 0.1 (0.7999999999999999)
against 0.8, such as the Laptop pair in threshold-gated-evaluation.md,
was an FD at those gates and a TP on a List[str] element.

Route every gate through ThresholdHelper.is_above_threshold. A score
inside the tolerance now counts as a TP, keeps its score instead of
being clipped to 0.0, and opens a list pair for field-by-field
evaluation. Scores further under the threshold are unchanged.

HungarianMatcher imports the helper inside calculate_metrics, so
stickler.algorithms does not load the evaluator package at import time.

Fixes awslabs#417
@Thanatos9404
Thanatos9404 requested a review from sromoam as a code owner October 18, 2026 10:07
@sromoam sromoam added this to the 1.1.0 milestone Oct 19, 2026
@sromoam

sromoam commented Oct 19, 2026

Copy link
Copy Markdown
Contributor

Thanks @Thanatos9404, this is solid work. I reviewed it and found no bugs in the code: every evaluator gate I checked now uses the tolerant test, and the new tests pass.

#417 is in the 1.1.0 milestone, and we're holding 1.0.1 to what's already in flight, so I've moved this PR to 1.1.0. We'll pick it up right after 1.0.1 ships. If you want to get ahead of it, these are the items I'd want before merging:

  1. Docs that still state the bare >= rule. EvalResult.matched is documented as overall_score >= match_threshold in API-Reference/auto.md:52, Guides/Evaluation/understanding-results.md:49 and threshold-gated-evaluation.md:206. The field verdict is similarity >= threshold in Getting-Started/thresholds-and-metrics.md:41, hungarian-matching.md:33 and :198, classification-logic.md:42 and :104, and models/confidence/README.md:28. A sentence on the tolerance in one place, linked from the others, would do.
  2. The "Good Match" Laptop example in threshold-gated-evaluation.md:101. It still shows name as a TP at about 0.9, but your test asserts it's an FD (Levenshtein gives 0.4) with a products[0].name non-match.
  3. "At every gate" in classification-logic.md:197 and the CHANGELOG. BaseComparator.binary_compare and ComparisonInfo.compare / ComparableFieldConfig.compare still use bare >=. Either switch them too or narrow the claim.
  4. src/stickler/algorithms/README.md:90 still describes a difference between calculate_metrics and unordered_list_metrics that this PR removes.

Optional: structured_list_comparator.py:137 and hungarian.py:330 could call ThresholdHelper.apply_threshold_logic instead of re-counting. If the predicate lived in a dependency-free module, hungarian.py wouldn't need the deferred import either. Neither one blocks the merge.

BaseComparator.binary_compare, ComparisonInfo.compare and
ComparableFieldConfig.compare still used a bare comparison, so 0.7 + 0.1
against 0.8 was (0, 1) from binary_compare and was clipped to 0.0 by the
field configs. They now go through ThresholdHelper, as does the unused
StructuredModel._calculate_object_level_metrics. binary_compare imports
the helper inside the method, like HungarianMatcher.calculate_metrics.

Add a Meeting a Threshold section to thresholds-and-metrics.md and link
it from each page that states the >= rule. Correct the Good Match Laptop
scenario, where name scores 0.4 and is an FD inside a TP pair, and the
algorithms README note on unordered_list_metrics. The CHANGELOG entry
names the new gates and the two thresholds that keep an exact
comparison.
@Thanatos9404

Copy link
Copy Markdown
Contributor Author

Thanks for the careful review. bd338aa adds a short "Meeting a Threshold" section to thresholds-and-metrics.md and links it from each page you listed (plus Guides/Evaluation/README.md and the architecture page), fixes the Laptop example so name is an FD at 0.4 inside a TP pair with products[0].name in non_matches, and updates the algorithms README. binary_compare, ComparisonInfo.compare and ComparableFieldConfig.compare now use the same tolerant test, with a new test at 0.7 + 0.1 against 0.8, and the CHANGELOG names the two thresholds that stay exact (the ANLS* leaf cutoff and the bounding box IoU thresholds). I left the two optional refactors out to keep this one focused, and I noticed the Result Structure counts further down that page were already off before this PR (with 3 GT and 3 predictions, Hungarian pairs Cable with New Product as an FD), so I can do either of those in a separate PR if you like.

@sromoam

sromoam commented Oct 20, 2026

Copy link
Copy Markdown
Contributor

Thanks @Thanatos9404, nice turnaround. I re-checked bd338aa. The full suite passes, and so does test_threshold_tolerance.py (54 passed). Three of the four items are done, and the fourth is mostly done:

  1. Bare >= wording: done. All eight places link to the new "Meeting a Threshold" section.
  2. Laptop example: the pair itself is now right (a TP, with name an FD at 0.4). The rest of that worked example still needs a pass, see below.
  3. "At every gate": done. binary_compare, ComparisonInfo.compare and ComparableFieldConfig.compare use the tolerant test, and the claim is scoped to the listed gates.
  4. algorithms/README.md: done.

Before we merge this in 1.1.0:

  • threshold-gated-evaluation.md:126 and the "Unmatched Items" scenario: with 3 ground-truth and 3 predicted products, every item is paired, so there is no FN or FA. Running the page's own Order/Product example on this branch gives products tp=1, fd=2, fn=0, fa=0, Mouse at 0.6725 (not 0.3), and Cable vs New Product as a paired FD at 0.133. The page shows 1/1/1/1 and cm_precision 0.5 (actual 1/3). Regenerating the block from a real run would fix it, and a test on the whole example would keep it fixed.
  • Docstrings that say "the one every evaluator gate uses" (hungarian.py:293, base.py:244, algorithms/README.md): the ANLS* leaf threshold and bbox IoU stay exact, as your CHANGELOG says. "Every classification gate listed in Meeting a Threshold" would match.
  • The heading "Good Match (similarity >= 0.8)" (threshold-gated-evaluation.md:93) sits over a 0.7999999999999999 example. "Good Match (meets 0.8)" avoids the contradiction.

Optional: facade.py:144 still uses a bare raw_sim < threshold for the explain verdict, which can't misfire today because of the score == 0.0 guard. ThresholdHelper.is_below_threshold would keep it consistent. The predicate move and the apply_threshold_logic reuse are still welcome but not needed.

One thing on our side, not yours: this is marked Breaking and milestoned 1.1.0, and RELEASING.md says breaking changes go in a major. We'll sort out the release labelling before it ships.

Keep both import sets. The list reporting work on dev passes scorer
verdicts down to the reports; build them with
ThresholdHelper.is_above_threshold in StructuredListComparator, use it for
the fallback in process_matched_pairs, and compare against it when
FieldComparisonHelper explains a field whose element verdicts disagree
with its score.

Two list reporting tests pinned the old exact scalar threshold. A list
item's scalar sub-field at 0.7 - 1e-12 against 0.7 is now a TP, and a
list field at that score is explained as within the tolerance.
The Result Structure block and the Unmatched Items scenario in
threshold-gated-evaluation.md did not match what the page's own example
produces. With three products on each side, Hungarian matching pairs
Cable with New Product as an FD at 0.133, so there is no FN or FA, and
the Mouse pair scores 0.6725. Show the example inputs, replace the block
with the real counts and non_matches, retitle the heading over the
0.7999999999999999 pair, and say when an item is left unmatched. The
Laptop test now runs the whole example, plus the FN and FA cases.

Scope the docstrings in hungarian.py, base.py and the algorithms README
to the gates listed in Meeting a Threshold, and use
ThresholdHelper.is_below_threshold for the explain() verdict.
@Thanatos9404

Copy link
Copy Markdown
Contributor Author

Thanks for checking it again. 0d971ca puts the example inputs on the page and replaces the Result Structure block with the counts and non_matches from a real run (tp=1, fd=2, Mouse at 0.6725, Cable and New Product as a paired FD at 0.133), and the Unmatched Items scenario now explains that an item is only left unmatched when one list is longer. The test now runs the whole example, plus the FN and FA cases you get by dropping one item. The docstrings now point at the gates listed in Meeting a Threshold, the heading reads "Good Match (meets 0.8)", and explain() uses is_below_threshold. I also merged dev (d19dc9b): the new list reporting verdicts go through the same helper, and two tests from #364 that pinned the exact scalar threshold now expect the tolerance.

Thanatos9404 and others added 2 commits October 29, 2026 01:21
# Conflicts:
#	CHANGELOG.md
#	src/stickler/auto/facade.py
#	src/stickler/structured_object_evaluator/models/structured_model.py
@Thanatos9404

Copy link
Copy Markdown
Contributor Author

Thanks for merging dev in. I checked the result: facade.py still uses the tolerant test in both places, and the structured_model.py hunk dropping out is expected, since #398 removed _calculate_object_level_metrics. None of the newer dev changes add a bare >= threshold check.

@github-actions

github-actions Bot commented Oct 29, 2026 •

Copy link
Copy Markdown

Security Scan Results

ASH Security Scan Report

  • Report generated: 2026-10-28T22:50:24+00:00
  • Time since scan: 3 minutes

Scan Metadata

  • Project: ASH
  • Scan executed: 2026-10-28T22:46:40+00:00
  • ASH version: 3.1.2

Summary

Scanner Results

The table below shows findings by scanner, with status based on severity thresholds and dependencies:

  • Severity levels:
    • Suppressed (S): Findings that have been explicitly suppressed and don't affect scanner status
    • Critical (C): Highest severity findings that require immediate attention
    • High (H): Serious findings that should be addressed soon
    • Medium (M): Moderate risk findings
    • Low (L): Lower risk findings
    • Info (I): Informational findings with minimal risk
  • Duration (Time): Time taken by the scanner to complete its execution
  • Actionable: Number of findings at or above the threshold severity level that require attention
  • Result:
    • PASSED = No findings at or above threshold
    • FAILED = Findings at or above threshold
    • MISSING = Required dependencies not available
    • SKIPPED = Scanner explicitly disabled
    • ERROR = Scanner execution error
  • Threshold: The minimum severity level that will cause a scanner to fail
    • Thresholds: ALL, LOW, MEDIUM, HIGH, CRITICAL
    • Source: Values in parentheses indicate where the threshold is set:
      • global (global_settings section in the ASH_CONFIG used)
      • config (scanner config section in the ASH_CONFIG used)
      • scanner (default configuration in the plugin, if explicitly set)
  • Statistics calculation:
    • All statistics are calculated from the final aggregated SARIF report
    • Suppressed findings are counted separately and do not contribute to actionable findings
    • Scanner status is determined by comparing actionable findings to the threshold
Scanner Suppressed Critical High Medium Low Info Actionable Result Threshold
bandit 0 4 0 0 6396 0 4 FAILED MEDIUM (global)
cdk-nag 0 0 0 0 0 0 0 PASSED MEDIUM (global)
cfn-nag 0 0 0 0 0 0 0 MISSING MEDIUM (global)
checkov 0 0 0 0 0 0 0 PASSED MEDIUM (global)
detect-secrets 0 1 0 0 0 0 1 SKIPPED MEDIUM (global)
grype 0 0 0 0 0 0 0 MISSING MEDIUM (global)
npm-audit 0 0 0 0 0 0 0 PASSED MEDIUM (global)
opengrep 0 0 0 0 0 0 0 MISSING MEDIUM (global)
semgrep 3 0 0 0 0 0 0 PASSED MEDIUM (global)
syft 0 0 0 0 0 0 0 MISSING MEDIUM (global)

Top 3 Hotspots

Files with the highest number of security findings:

Finding Count File Location
2 tests/comparators/test_anls_leaf_semantics.py
2 tests/structured_object_evaluator/test_rollup_node_semantics.py
1 tests/test_readme_examples.py

Detailed Findings

Show 5 actionable findings

Finding 1: SECRET-HEX-HIGH-ENTROPY-STRING

  • Severity: HIGH
  • Scanner: detect-secrets
  • Rule ID: SECRET-HEX-HIGH-ENTROPY-STRING
  • Location: tests/comparators/test_anls_leaf_semantics.py:89

Description:
Secret of type 'Hex High Entropy String' detected in file 'tests/comparators/test_anls_leaf_semantics.py' at line 89

Code Snippet:

Secret of type Hex High Entropy String detected

Finding 2: B108

  • Severity: HIGH
  • Scanner: bandit
  • Rule ID: B108
  • Location: tests/comparators/test_anls_leaf_semantics.py:210-212

Description:
Probable insecure usage of temp file/directory.

Code Snippet:

uuid.UUID(int=1),
            pathlib.Path("/tmp/x"),
            b"bytes",

Finding 3: B307

  • Severity: HIGH
  • Scanner: bandit
  • Rule ID: B307
  • Location: tests/structured_object_evaluator/test_rollup_node_semantics.py:1363-1369

Description:
Use of possibly insecure function - consider using safer ast.literal_eval.

Code Snippet:

def published_label(node: dict, section: str, object_lists: set) -> str:
            counts_objects = eval(  # noqa: S307
                cond.group(1),
                {},
                {"data": node, "section": section, "OBJECT_LISTS": object_lists},
            )
            return eval(unit.group(1), {}, {"counts_objects": counts_objects})  # noqa: S307

Finding 4: B307

  • Severity: HIGH
  • Scanner: bandit
  • Rule ID: B307
  • Location: tests/structured_object_evaluator/test_rollup_node_semantics.py:1368-1370

Description:
Use of possibly insecure function - consider using safer ast.literal_eval.

Code Snippet:

)
            return eval(unit.group(1), {}, {"counts_objects": counts_objects})  # noqa: S307

Finding 5: B102

  • Severity: HIGH
  • Scanner: bandit
  • Rule ID: B102
  • Location: tests/test_readme_examples.py:55-57

Description:
Use of exec detected.

Code Snippet:

with contextlib.redirect_stdout(out):
        exec(code, namespace)
    prints = [line for line in code.splitlines() if line.startswith("print(")]

Report generated by Automated Security Helper (ASH) at 2026-10-28T22:50:26+00:00

@sromoam sromoam left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Thanatos9404. I re-checked f3acaa7: test_threshold_tolerance.py and the full suite pass, and no exact verdict gate remains outside ANLS and IoU. Approving. Please update the description before merge: it still lists binary_compare, ComparisonInfo.compare, ComparableFieldConfig.compare and explain() as left alone, but this PR changes all four. The remaining reason-text and stale >= cleanups are filed as #462.

@sromoam
sromoam merged commit 4a6ef00 into awslabs:dev Oct 29, 2026
10 checks passed
@Thanatos9404

Copy link
Copy Markdown
Contributor Author

Updated the description: binary_compare, both compare methods and explain() are now listed under the changes, and the left-alone list is down to the ANLS* leaf threshold and bbox IoU. Thanks for the careful reviews, and for filing #462.

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.

Use one threshold tolerance at every classification gate

2 participants