Repository navigation
fix(417)!: use one threshold tolerance at every classification gate - #421
Conversation
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
|
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:
Optional: |
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.
|
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 |
|
Thanks @Thanatos9404, nice turnaround. I re-checked
Before we merge this in 1.1.0:
Optional: 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.
|
Thanks for checking it again. 0d971ca puts the example inputs on the page and replaces the Result Structure block with the counts and |
# Conflicts: # CHANGELOG.md # src/stickler/auto/facade.py # src/stickler/structured_object_evaluator/models/structured_model.py
|
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 |
Security Scan ResultsASH Security Scan Report
Scan Metadata
SummaryScanner ResultsThe table below shows findings by scanner, with status based on severity thresholds and dependencies:
Top 3 HotspotsFiles with the highest number of security findings:
Detailed FindingsShow 5 actionable findingsFinding 1: SECRET-HEX-HIGH-ENTROPY-STRING
Description: Code Snippet: Finding 2: B108
Description: Code Snippet: Finding 3: B307
Description: Code Snippet: Finding 4: B307
Description: Code Snippet: Finding 5: B102
Description: Code Snippet: Report generated by Automated Security Helper (ASH) at 2026-10-28T22:50:26+00:00 |
sromoam
left a comment
There was a problem hiding this comment.
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.
|
Updated the description: |
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 to1e-10under its threshold now gets the same verdict wherever it is classified.Changes Made
Line numbers are at
003b5f1.field_comparator.py:68and: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:136and:247: theList[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 behindnon_matchesandfield_comparisons.field_comparison_collector.py:140andfield_comparison_helper.py:136: thematchflag onfield_comparisonsrows, 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 samematch_thresholdas list pairing.metrics_helper.py:145: the binary metricsevaluator_format=Truefalls back to without a confusion matrix.BaseComparator.binary_compare,ComparisonInfo.compareandComparableFieldConfig.compare(the clip incomparison_info.py), added during review so they agree with the evaluator gates.EvalResult.explain()now reports a clip withis_below_threshold, the same test.algorithms/hungarian.py:325:HungarianMatcher.calculate_metricstp. The evaluator only readsmatched_pairsfrom it, but its docstring andalgorithms/README.mddescribed 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 importshungarian.pywhile it initializes.classification-logic.mdandthreshold-gated-evaluation.md; the trap removed from theHungarianMatcherdocstring andalgorithms/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.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
devat003b5f1, with0.7 + 0.1(0.7999999999999999) against0.8:devList[str]elementfield_comparisonsreasonbelow threshold (0.800 < 0.8)List[StructuredModel]pair (the Laptop pair inthreshold-gated-evaluation.md)products[0]non-matchproduct_idTP,nameFD (0.4 against 0.7),priceTPitems[0].namenon-matchEvalResult.matchedatoverall_score0.7999999999999999FalseTrueHungarianMatcher.calculate_metricstp0,fd1tp1,fd0A score further under the threshold is unchanged:
0.8 - 1e-9and0.79are FDs before and after.Left as they are, on purpose:
trees/leaf_tree.py:140: the ANLS* tree behindanls_score, a separate metric.bbox/calculator.py:348: an IoU threshold, not a similarity score.StructuredModel._calculate_object_level_metrics: refactor(395): remove the remaining dead confusion-matrix paths #398 deleted it, so there was nothing to change.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.pychange here dead; I'll drop it on rebase if #406 lands first. I'll rebase on whichever lands first.Testing
signal.SIGALRM; details below)test_threshold_tolerance.pychecks each gate at four scores: one rounding error under the threshold, exactly at it,1e-9under it (outside the tolerance) and0.79. For each gate it checks the counts and, where the gate feeds them, the clipped score,non_matchesandfield_comparisons. One test checks that a list item sub-field, a top-level field and aList[str]element give the same counts for the same score, and one uses the documented Laptop pair as is. 50 tests: ondev, 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'ssrc/stickler/__init__.py.uv run pytest -n auto -qdev003b5f1The 6 failures are the same on both trees: 5 tests in
TestRealisticDeepNestingandtest_integrationintest_realistic_deep_nesting.py, allAttributeError: module 'signal' has no attribute 'SIGALRM'. Linux CI runs them.uv run ruff checkpasses on the changed Python files.ruff format --checkis clean forhungarian.py,facade.py,confusion_matrix_calculator.py,metrics_helper.pyand the new test file; the other five were already offruff formatondev, and none of the drifting lines are ones this PR touches.Reviewers
@sromoam
Checklist
CHANGELOG.mdentry added under[Unreleased], or not needed becausenothing user-visible changed (see RELEASING.md)
Additional Notes
1e-10, asThresholdHelperalready had it). I did not add a constant or a new helper; every gate calls the existing method.devbefore 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.