Feat/universal aggregate metrics#115
Conversation
* Mainly, thresholding gating logic applicable to object level metrics should not affect aggregation metric. The threshold gated evaluation is edited according. * Also, the aggregate-metrics documentation is updated to capture more details on how aggregate is computed for each node type.
… after "Key Features", before the two examples. "Calculation Summary" follows the examples. Replaced the confusing shorthand bullet list in Example 2 with a table that maps each item (AAA/BBB/CCC) to its object-level classification and per-field outcomes, then shows how the column sums produce items.aggregate. Updated the notebook to match.
…model_evaluator to optionally include aggregate results and optionally use recall with fd
…rics to handle aggregation of good and bad matches. The logic is changed from reliance on _handle_hierarchical_field to compare_recursive in comparison_engine.
…turning the field_details. This will allow the aggregate metrics to correctly trickle up.
1) In comparison_dispatcher.py, inside dispatch_field_comparison function, use handle_list_field_dispatch only when simple list. When it's structured list, let compare_struct_list_with_scores handle structured list. 3) In structured_list_comparator.py, _calculate_nested_field_metrics, simplied the code. No threshold of matched pairs is applied. Field details are calculated for all matched pairs and matched objects. Note that the following update is no longer applies as the method is removed in dev: 2) In structured_list_comparator.py, in _handle_struct_list_empty_cases only handle when both are empty. Otherwise, run through the logic. This is necessary to get the field level metrics.
… (e.g., when field value is list versus string) 2) Removed checking for _aggregate flag in test_json_schema_field_converter.py 3) One major change in many test files:
* Update incorrect test cases to pass all test cases in test_structured_model_evaluator_nested.py. Some test cases still fail in test_ecommerce_orders_aggregate_comprehensive.py, which require additional code change. Clean up some print statements, unused tests. * Update comments per feedback --------- Co-authored-by: Hayley Park <parhyunj@amazon.com>
* Updating the logic in dispatcher: 1) Moving from runtime type checking to static field definition checking. Doing this by moving away from _should_use_hierarchical_structure and using _is_structured_list_field instead. This is done mainly for list fields, both primitive and structured. Future changes could expand to other types as needed. 2) when the field is a structured list, delegate handling all types of values whether null or not, to StructuredListComparator Updated structured_model: Add helper method to check if a field is specifically List[StructuredModel], complementing _is_list_field() which checks for any list type.
* Update the early exit logic in Hungarian matching to make sure there is always matched pairs for aggregate metrics --------- Co-authored-by: Hayley Park <parhyunj@amazon.com>
* Previously when a structured list was empty, it would be counted as one TN for aggregate at the object level but aggregation may miss this (because overall metrics are skipped when there are child instances). This change creates a field dict to capture TN at the child leaf node, so that aggregate metric can trickle up correctly. 2) Updated tests to reflect changes due to above logic
* Update hungarian to better handle single-item lists. Always match single-item lists, count as tp if the item similarity score is higher than match_threshold. Update fn count for a test_hungarian test case * Remove an unused, commented out variable
…per.is_effectively_null_for_primitives in comparison_dispatcher
…ich is always `True`
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 1 HotspotsFiles with the highest number of security findings:
Detailed FindingsShow 1 actionable findingsFinding 1: CKV2_GHA_1
Description: Report generated by Automated Security Helper (ASH) at 2026-04-24T21:43:44+00:00 |
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 1 HotspotsFiles with the highest number of security findings:
Detailed FindingsShow 1 actionable findingsFinding 1: CKV2_GHA_1
Description: Report generated by Automated Security Helper (ASH) at 2026-04-24T21:46:09+00:00 |
|
Continuing the effort from #72. Update documentation along with the code change |
…s()` by changing direct dictionary key access (`metrics["tp"]`) to use `.get()` with a default of 0 (`metrics.get("tp", 0)`) for all six confusion matrix keys (tp, fp, tn, fn, fd, fa). This handles the edge case where `aggregate_from_comparisons([])` is called with an empty list — no documents get processed, so the confusion matrix dict remains empty, and the derived metrics calculation now gracefully returns zeros instead of crashing. All 26 tests in the file now pass.
…he "matched" count, producing FN+FA instead of FD - __Field-level recursion:__ All Hungarian-paired items still get their fields compared (preserving existing FD behavior at the aggregate/field level for partial matches)
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 1 HotspotsFiles with the highest number of security findings:
Detailed FindingsShow 1 actionable findingsFinding 1: CKV2_GHA_1
Description: Report generated by Automated Security Helper (ASH) at 2026-04-30T23:02:58+00:00 |
|
3 test cases were failing with the changes + the new dev:
Changes pushed to fix the issues:
|
sromoam
left a comment
sromoam
left a comment
There was a problem hiding this comment.
Sorry, I thought these comments were already posted.
The behavior of aggregates should not change the behavior of the overall. I agree with the deisgn of the aggregates, but I don't agree with the changes in some of the test assertions. Let's find time to discuss.
| # Should have nested fields for aggregate metrics | ||
| assert (len(nested_fields) > 0), "Should have nested fields for aggregate metrics" | ||
|
|
||
| # Check that overall sections are empty (no matches above threshold) | ||
| for field_name, field_metrics in nested_fields.items(): | ||
| overall_metrics = field_metrics["overall"] | ||
| aggregate_metrics = field_metrics["aggregate"] | ||
|
|
||
| # Overall should have some metrics from poor matches at the leaf node level. | ||
| # When the node is primitive or simple list, overall metrics will be same as aggregate metrics. | ||
| # When node is structured list, then overall can be different from aggregate due to threshold. | ||
| overall_total = sum(overall_metrics[metric] for metric in ["tp", "fa", "fd", "fp", "tn", "fn"]) | ||
| assert (overall_total > 0), f"Field {field_name} overall should not be empty for poor matches" | ||
|
|
||
| # Aggregate should have some metrics from poor matches | ||
| aggregate_total = sum(aggregate_metrics[metric] for metric in ["tp", "fa", "fd", "fp", "tn", "fn"]) | ||
| assert (aggregate_total > 0), f"Field {field_name} aggregate should have metrics from poor matches" |
There was a problem hiding this comment.
I'm glad we're testing these counts. Could we refactor this to actually test that for primitives we get the same, but for list[StructuredModel] we get different?
| assert ( | ||
| get_metric(cm["fields"]["pets"]["fields"]["petId"], "tp") == 1 | ||
| ), "Expected 1 true positives" | ||
| get_metric(cm["fields"]["pets"]["fields"]["petId"], "tp") == 2 | ||
| ), "Expected 2 true positives" | ||
| assert ( |
There was a problem hiding this comment.
This assertion should not have changed. Max the dog entity falls below the set threshold, so for the 'overall' key, it should still be 1 not 2. Becuase Max is considered a bad match, therfore no field in there is TP.
There was a problem hiding this comment.
This applies to the rest of the tests in this file.
There was a problem hiding this comment.
Thank you for pointing this out. I updated relevant code and test cases not to update the overall metrics, but only update aggregate metrics.
…ly above-threshold matched pairs (+ unmatched FN/FA items) contribute; 2. "aggregate": ALL pairs contribute, regardless of threshold. Revert incorrect test changes.
|
Latest commit 93db235 separates threshold-gating into two paths (per-field "overall" and "aggregate" to preserve "overall" behavior and reverts incorrect test changes. |
|
Reverted changes to src/stickler/structured_object_evaluator/bulk_structured_model_evaluator.py to keep the PR focused on aggregate metrics. Will update the file in the future if needed. |
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 1 HotspotsFiles with the highest number of security findings:
Detailed FindingsShow 1 actionable findingsFinding 1: CKV2_GHA_1
Description: Report generated by Automated Security Helper (ASH) at 2026-05-15T21:04:55+00:00 |
…uctured_model_evaluator.py to keep the PR focused on aggregate metrics.
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 1 HotspotsFiles with the highest number of security findings:
Detailed FindingsShow 1 actionable findingsFinding 1: CKV2_GHA_1
Description: Report generated by Automated Security Helper (ASH) at 2026-05-15T21:47:16+00:00 |
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 7 HotspotsFiles with the highest number of security findings:
Detailed FindingsShow 19 actionable findingsFinding 1: yaml.github-actions.security.github-actions-mutable-action-tag.github-actions-mutable-action-tag
Description: Code Snippet: Finding 2: yaml.github-actions.security.github-actions-mutable-action-tag.github-actions-mutable-action-tag
Description: Code Snippet: Finding 3: yaml.github-actions.security.github-actions-mutable-action-tag.github-actions-mutable-action-tag
Description: Code Snippet: Finding 4: yaml.github-actions.security.github-actions-mutable-action-tag.github-actions-mutable-action-tag
Description: Code Snippet: Finding 5: yaml.github-actions.security.github-actions-mutable-action-tag.github-actions-mutable-action-tag
Description: Code Snippet: Finding 6: yaml.github-actions.security.github-actions-mutable-action-tag.github-actions-mutable-action-tag
Description: Code Snippet: Finding 7: yaml.github-actions.security.github-actions-mutable-action-tag.github-actions-mutable-action-tag
Description: Code Snippet: Finding 8: yaml.github-actions.security.github-actions-mutable-action-tag.github-actions-mutable-action-tag
Description: Code Snippet: Finding 9: yaml.github-actions.security.github-actions-mutable-action-tag.github-actions-mutable-action-tag
Description: Code Snippet: Finding 10: yaml.github-actions.security.github-actions-mutable-action-tag.github-actions-mutable-action-tag
Description: Code Snippet: Finding 11: yaml.github-actions.security.github-actions-mutable-action-tag.github-actions-mutable-action-tag
Description: Code Snippet: Finding 12: yaml.github-actions.security.github-actions-mutable-action-tag.github-actions-mutable-action-tag
Description: Code Snippet: Finding 13: yaml.github-actions.security.github-actions-mutable-action-tag.github-actions-mutable-action-tag
Description: Code Snippet: Finding 14: yaml.github-actions.security.github-actions-mutable-action-tag.github-actions-mutable-action-tag
Description: Code Snippet: Finding 15: yaml.github-actions.security.github-actions-mutable-action-tag.github-actions-mutable-action-tag
Description: Code Snippet: Finding 16: yaml.github-actions.security.github-actions-mutable-action-tag.github-actions-mutable-action-tag
Description: Code Snippet: Finding 17: yaml.github-actions.security.github-actions-mutable-action-tag.github-actions-mutable-action-tag
Description: Code Snippet: Finding 18: yaml.github-actions.security.github-actions-mutable-action-tag.github-actions-mutable-action-tag
Description: Code Snippet: Finding 19: package_managers.uv.uv-missing-dependency-cooldown.uv-missing-dependency-cooldown
Description: Code Snippet: Report generated by Automated Security Helper (ASH) at 2026-07-16T22:12:17+00:00 |
…sons export The dev merge overwrote the v0.4.0 bulk evaluator with an older version missing weighted_overall_score, persistent JSONL handle, accumulator error isolation, and update_from_comparison_result(). Restore from main and add missing imports (LevenshteinComparator, NumericComparator, math) to the test file.
…aggregate rollup Implements the missing piece from PR #115: BulkStructuredModelEvaluator now rolls up per-document `cm['aggregate']` blocks into corpus-level totals via a plug-in accumulator, mirroring the existing ConfidenceAccumulator pattern. The accumulator is added to the default accumulators list, so corpus-level aggregate metrics are on by default. Users access them via ProcessEvaluation.accumulator_metrics["aggregate_metrics"] with the familiar {overall, fields} shape — overall holds summed tp/fd/fa/fn/tn counts plus derived precision/recall/f1/accuracy, fields keys flat dotted paths (e.g. "items.sku") with the same shape. Implementation - New module: src/stickler/structured_object_evaluator/models/aggregate/ with __init__.py and accumulator.py defining AggregateConfusionMatrixAccumulator(PostComparisonAccumulator). - accumulate() reads comparison_result["confusion_matrix"], skips silently if absent (i.e. include_confusion_matrix=False), then sums the top-level "aggregate" block plus walks fields/nested_fields recursively to flatten dotted paths. - compute() returns None if no documents contributed; otherwise emits the {overall, fields} shape with derived metrics computed at each level via MetricsHelper.calculate_derived_metrics. - get_state/load_state/merge_state implemented for distributed eval and checkpointing; merge_state is additive on overall + per-field counts. - Wired into BulkStructuredModelEvaluator default accumulators list at bulk_structured_model_evaluator.py:134-137 alongside ConfidenceAccumulator. Tests (+21 net new) - tests/structured_object_evaluator/test_aggregate_accumulator.py (14 unit tests): ABC conformance, reset, empty compute, single/multi document accumulation, dotted-path flattening at 2 and 3 levels, derived metrics presence, missing confusion_matrix and aggregate keys handled gracefully, state round-trip via JSON, merge_state additivity. - tests/structured_object_evaluator/test_bulk_aggregate_integration.py (7 integration tests): default-includes-aggregate, parity test (manually summed per-doc aggregate == bulk rollup), threshold-gated FD pair recursion at corpus scale, distributed-eval merge_state round-trip, opt-out via custom accumulators list, empty-evaluation no-crash. - test_confidence_metrics.py: one assertion updated to match the new default accumulators list (was len==1, now ordered name list of 2). Documentation - aggregate-metrics.md: flipped the "Bulk evaluator does not yet aggregate" pitfall into a positive "Corpus-level aggregate metrics (bulk evaluation)" section with a code snippet showing ProcessEvaluation.accumulator_metrics["aggregate_metrics"] access. - aggregate-metrics.md: added a "Bulk evaluation" subsection under "Node Types and Aggregation Behavior" introducing the plug-in accumulator pattern and naming both built-ins. - aggregate-metrics.md: new Example 3 "Corpus-level Rollup with BulkStructuredModelEvaluator" walking through 3 documents (TP-only, FD pair, unmatched FN) and showing the corpus-level rollup output. - examples/notebooks/Aggregate_metrics.ipynb: new Example 3 section using the same fixture as the doc example, with corpus rollup and per-field flat-path output. Also includes a section on opting out or extending the default accumulator set. Notebook executes cleanly end-to-end and reproduces the doc's published numbers exactly. Test suite: 1121 passed / 2 skipped (was 1100; +21 net new tests; 0 regressions).
Fixes documentation accuracy and one regression caught during review.
Adds boundary-case tests that pin invariants the docs claim but were
not exercised before. All test deltas reflect actual code behavior;
no behavior loosening.
Documentation
- aggregate-metrics.md: added a "Why aggregates?" motivation section
that contrasts overall (object-level, threshold-gated) with
aggregate (field-level rollup across all items) so readers see the
problem the feature solves before diving into mechanics.
- aggregate-metrics.md Example 2: per-field `overall` JSON corrected
to {tp:1, fd:0, fn:1} for sku/description/qty (the BBB FD pair is
threshold-gated out of `overall` but still appears in `aggregate`).
Added prose contrasting `overall` vs `aggregate` so the example is
coherent end-to-end.
- aggregate-metrics.md: appended a "Common pitfalls" section with
five linkable subsections (overall vs aggregate divergence,
zero-similarity pair handling, empty-list semantics, bulk evaluator
not yet rolling up aggregate, recall_with_fd flag).
- threshold-gated-evaluation.md "Result Structure" worked example:
reframed from 3 GT vs 3 Pred (which Hungarian fully pairs, making
the documented {tp:1, fd:1, fn:1, fa:1} unreachable) to 3 GT vs
2 Pred. Updated counts and derived metrics to match what the code
actually emits.
- structured_list_comparator.py module docstring: replaced the
outdated "Current Behavior Preserved (including bugs)" block with
an accurate description of post-PR behavior.
Code
- hungarian.py: restored the `if score > 0:` gate in
HungarianMatcher.calculate_metrics' single-item-list shortcut so
zero-similarity pairs decompose as FN+FA, matching dev/main and
consistent with the >0.0 filter in StructuredListComparator.
Tests
- test_threshold_gated_recursion.py adds
test_zero_similarity_pair_is_unmatched_not_fd, pinning the
`> 0.0` boundary at structured_list_comparator.py:209. Uses a 2x2
non-overlapping container so Hungarian's pair-assignment branch is
actually exercised; mutation-tested by flipping > to >= (test fails).
- test_aggregate_cases.py adds
test_primitive_list_zero_similarity_treated_as_unmatched as a
regression test for the hungarian.py single-item fix.
- test_aggregate_cases.py: 7 of 12 TestAggregation tests now also
assert derived metrics (cm_precision, cm_recall, cm_f1, cm_accuracy)
under agg_results['derived'], pinning the docs claim that derived
metrics are recomputed at each aggregate level.
- test_ecommerce_orders_aggregate_comprehensive.py
test_threshold_based_classification: replaced two tautology
assertions (tp > 0; sum > 0) with concrete per-child overall and
aggregate counts that pin threshold behavior at every level.
- test_ecommerce_orders_aggregate_comprehensive.py: comment claiming
"5 TP, 1 TN" updated to match the asserted tp=4/tn=2; removed an
orphan __main__ call to a non-existent test method.
Test suite: 1100 passed / 2 skipped (was 1098 / 2, +2 from new tests).
No regressions; mutation-guard verified on the new boundary test.
…aggregate rollup Implements the missing piece from PR #115: BulkStructuredModelEvaluator now rolls up per-document `cm['aggregate']` blocks into corpus-level totals via a plug-in accumulator, mirroring the existing ConfidenceAccumulator pattern. The accumulator is added to the default accumulators list, so corpus-level aggregate metrics are on by default. Users access them via ProcessEvaluation.accumulator_metrics["aggregate_metrics"] with the familiar {overall, fields} shape — overall holds summed tp/fd/fa/fn/tn counts plus derived precision/recall/f1/accuracy, fields keys flat dotted paths (e.g. "items.sku") with the same shape. Implementation - New module: src/stickler/structured_object_evaluator/models/aggregate/ with __init__.py and accumulator.py defining AggregateConfusionMatrixAccumulator(PostComparisonAccumulator). - accumulate() reads comparison_result["confusion_matrix"], skips silently if absent (i.e. include_confusion_matrix=False), then sums the top-level "aggregate" block plus walks fields/nested_fields recursively to flatten dotted paths. - compute() returns None if no documents contributed; otherwise emits the {overall, fields} shape with derived metrics computed at each level via MetricsHelper.calculate_derived_metrics. - get_state/load_state/merge_state implemented for distributed eval and checkpointing; merge_state is additive on overall + per-field counts. - Wired into BulkStructuredModelEvaluator default accumulators list at bulk_structured_model_evaluator.py:134-137 alongside ConfidenceAccumulator. Tests (+21 net new) - tests/structured_object_evaluator/test_aggregate_accumulator.py (14 unit tests): ABC conformance, reset, empty compute, single/multi document accumulation, dotted-path flattening at 2 and 3 levels, derived metrics presence, missing confusion_matrix and aggregate keys handled gracefully, state round-trip via JSON, merge_state additivity. - tests/structured_object_evaluator/test_bulk_aggregate_integration.py (7 integration tests): default-includes-aggregate, parity test (manually summed per-doc aggregate == bulk rollup), threshold-gated FD pair recursion at corpus scale, distributed-eval merge_state round-trip, opt-out via custom accumulators list, empty-evaluation no-crash. - test_confidence_metrics.py: one assertion updated to match the new default accumulators list (was len==1, now ordered name list of 2). Documentation - aggregate-metrics.md: flipped the "Bulk evaluator does not yet aggregate" pitfall into a positive "Corpus-level aggregate metrics (bulk evaluation)" section with a code snippet showing ProcessEvaluation.accumulator_metrics["aggregate_metrics"] access. - aggregate-metrics.md: added a "Bulk evaluation" subsection under "Node Types and Aggregation Behavior" introducing the plug-in accumulator pattern and naming both built-ins. - aggregate-metrics.md: new Example 3 "Corpus-level Rollup with BulkStructuredModelEvaluator" walking through 3 documents (TP-only, FD pair, unmatched FN) and showing the corpus-level rollup output. - examples/notebooks/Aggregate_metrics.ipynb: new Example 3 section using the same fixture as the doc example, with corpus rollup and per-field flat-path output. Also includes a section on opting out or extending the default accumulator set. Notebook executes cleanly end-to-end and reproduces the doc's published numbers exactly. Test suite: 1121 passed / 2 skipped (was 1100; +21 net new tests; 0 regressions).
… + close test gaps
Three follow-ups identified during PR review of the bulk-aggregate work:
1. recall_with_fd parameter (real fix). The accumulator was hardcoding
recall_with_fd=False when computing derived metrics, so corpus-level
users had no way to opt into the include-FD recall formula that
StructuredModel.compare_with already supports per-document. Adds an
optional recall_with_fd kwarg on AggregateConfusionMatrixAccumulator;
default remains False (matches per-document default), opt-in flips
the formula at every aggregate level (overall + per-field). Raw
counts are unchanged; only the derived block is affected.
2. Bulk evaluator state-lifecycle test gap. The accumulator's own
get_state/load_state/merge_state were unit-tested directly, but no
test exercised the path through BulkStructuredModelEvaluator.get_state
/ .load_state which is what distributed-eval consumers actually call.
Adds two integration tests:
- bulk get_state -> fresh evaluator load_state produces identical
aggregate_metrics
- bulk load_state followed by additional .update() calls additively
produces the same aggregate as a single-pass run
3. pretty_print_metrics gap. BulkStructuredModelEvaluator.pretty_print_metrics
was walking process_eval.metrics and process_eval.field_metrics but
ignoring accumulator_metrics, so the aggregate rollup was invisible
in the human-readable summary. Adds a new "AGGREGATE METRICS" section
to the printer surfacing corpus-level overall counts, derived metrics,
and per-field-path lines (sorted by F1 desc, mirroring the existing
FIELD-LEVEL METRICS layout). Confidence is also accumulator-only
today and equally invisible — left as a separate cleanup.
Tests added (+7)
- test_aggregate_accumulator.py::TestRecallWithFdParameter (4 tests):
default uses textbook recall, opt-in uses include-FD recall, the flag
propagates to per-field derived blocks, raw counts identical between
flavors.
- test_bulk_aggregate_integration.py::TestBulkEvaluatorStateLifecycle
(2 tests): get/load round-trip, load+continue equals single-pass.
- test_bulk_aggregate_integration.py::TestPrettyPrintSurfacesAggregate
(1 test): captures stdout, asserts AGGREGATE METRICS heading present.
Suite: 1128 passed / 2 skipped (was 1121, +7 net new tests).
adiadd
left a comment
adiadd
left a comment
There was a problem hiding this comment.
thanks for sticking with this one, the feature is the right direction but a few things worth resolving before merge, roughly in priority order:
blocking:
- per-field
overallstill changes vs dev in three nested scenarios (inner-list ungating, unmatched-item seeding, empty-vs-empty leaf seeding), which is the exact condition behind the standing changes-requested. see the comment onstructured_list_comparator.py:316for the verified side-by-side deltas.
worth resolving:
-
zero-similarity pairs now report FN+FA at object level but FD-shaped counts in per-field
aggregate, and vanish from per-fieldoverallentirely, see the comment atstructured_list_comparator.py:209. the doc-level metric change from that reclassification also still needs explicit sign-off on the thread. -
TestWeightedOverallScoreParitywas deleted but still passes at head, worth restoring as-is (comment ontest_bulk_evaluator_parity.py). -
class-level
_nesting_depthis mutable global state (comment atstructured_list_comparator.py:49), and ~290 lines of dead code from the rewrite are still in the file (comment at:460). -
the bulk evaluator default accumulator change is back after the thread said it was reverted, and the PR description is stale about
include_aggregates/recall_with_fd(comment onbulk_structured_model_evaluator.py:136). -
pretty-print labeling and rollup granularity (comment at
bulk_structured_model_evaluator.py:809).
appreciate it!
Blocking: - Remove _nesting_depth class state; gate per-field 'overall' at every nesting level (matching dev behavior and docs). This resolves the three per-field overall deltas (inner-list ungating, unmatched-item seeding, empty-vs-empty leaf seeding). Worth resolving: - Keep zero-similarity FN+FA reclassification (semantic correctness) and propagate aggregate keys through _recursive_merge_fields so inner list comparisons correctly surface in outer aggregate pre-seeding. - Restore TestWeightedOverallScoreParity (passes at head, deletion was unnecessary). - Remove ~290 lines of dead code: _handle_hierarchical_field, _handle_non_hierarchical_field, _add_derived_metrics_recursively, _should_use_hierarchical_structure, _get_all_leaves, _get_leaves_under_field, _get_nested_model_type, and stale references in comparison_dispatcher.py. - Fix pretty-print label: 'ungated by match_threshold' -> 'includes below-threshold structured list pairs'. Show TN-only fields. Add hierarchy note in comment. Nits: - Fix misleading test comment (describes aggregate, not overall). - Remove non-existent test_nested_category_description_aggregation from __main__ block. - Remove stale 'per the design spec' docstring reference; add comment about docs_with_data fallback behavior. Test assertion updates reflect the new correct behavior: - Empty-vs-empty lists emit single list-level TN=1 (no per-leaf expansion, matching dev). - Inner list aggregate keys are properly propagated through merging.
… name Rename references from _should_use_hierarchical_structure() to _is_structured_list_field() in the architecture documentation to reflect the current codebase method naming.
|
Thanks @adiadd for the thorough review. All items addressed in commit 0e51087: Blocking: resolved:
Worth resolving, all resolved:
All nits (misleading comment, non-existent main method, stale docstring reference) also fixed. |
adiadd
left a comment
adiadd
left a comment
There was a problem hiding this comment.
appreciate all the changes and fixes hayley! a couple findings still which i've outlined within some more comments
also, the branch currently conflicts with dev on uv.lock only (everything else auto-merges), so a rebase/regen should clear it, and the zero-sim FD to FN+FA reclassification could still use an explicit sign-off from @sromoam on the thread since it changes doc-level overall counts.
almost there!
| field_info and model_class._is_structured_field_type(field_info) | ||
| ) | ||
| #take all the tp values and convert to fn | ||
| field_details_tmp["fields"] = self._switch_metrics(field_details_tmp["fields"] , source_metric='tp', target_metric='fn') |
There was a problem hiding this comment.
blocking: so the unmatched-item seeding delta from my last review is still here, and it now cuts both ways: the unmatched item's nested-list FN/FA land in per-field overall (which dev keeps at zero) while going missing from aggregate (where #94 actually wants them).
reproducer: Top.mids -> Mid.leaves -> Leaf.name, GT [Mid("alpha", leaves=[l1, l2]), Mid("beta", leaves=[l3])] vs pred [Mid("beta", leaves=[l3])]. dev gives mids.fields.leaves.overall.fn=0 with no leaf entries; head gives leaves.fields.name overall {tp:1, fn:2} but aggregate {tp:1, fn:0}, and the root aggregate reports fn=1 instead of fn=3. symmetric on the pred side (fa/fp dropped). it's also input-dependent: with no matched pairs at all the leaf has no pre-seeded aggregate, the overall fallback fires, and the fn does get counted, so the same missing item counts or not depending on unrelated siblings.
mechanism, aggregate side: _switch_metrics builds a fresh target dict that only carries overall and fields keys (:371-397), so the aggregate keys the self-compare pre-seeded at inner leaves get dropped, then _preseed_aggregate_from_full prefers full["aggregate"] over full["overall"] at leaves (:428-431), and that aggregate was summed from matched pairs only. overall side: the gated_results.append for unmatched items at :345/:358 feeds the whole switched subtree (nested lists included) into the gated view, where dev only seeded the unmatched item's primitive fields.
suggested fix: have _switch_metrics also convert and propagate aggregate keys (tp to fn/fa/fp inside them, same as it does for overall) so unmatched contributions survive pre-seeding, and keep the nested-list part of the switched subtree out of gated_results so overall stays dev-shaped (the primitive seeding matches dev and can stay). this is exactly the visibility gap #94 was filed for, and the corpus accumulator inherits the inflated recall either way.
| "weight": weight, | ||
| } | ||
|
|
||
| case _: |
There was a problem hiding this comment.
two things on this branch: the comment says "Both non-empty" but case _ also matches (0, n) and (n, 0), and those fall through to hungarian matching on an empty list (which works, fa/fn come out right, hungarian_helper returns empty pairs without calling hungarian). worth the comment saying so.
the bigger note: for empty-vs-populated, this path now emits per-leaf overall rows that dev never emitted (e.g. GT [] vs 2 pred items yields items.fields.sku.overall fa=2/fp=2, dev just had the list-level counts). object and list level match dev exactly, and the seeding is consistent with how dev handles unmatched items in nonempty lists, so i'd say it's a defensible consistency fix, but it is a real per-field overall delta vs dev and it isn't in the PR description. given the standing concern about overall changes, worth calling it out explicitly on the thread so @sromoam can sign off rather than discover it later.
related shape note: below-threshold inner lists now emit all-zero leaf overall shells where dev omitted the keys entirely (dev: parts.fields = {}, head: parts.fields.name.overall all zeros). counts agree, but anything iterating fields keys or asserting exact dict shapes sees new entries, worth a line in the release notes.
| if gt_val and isinstance(gt_val[0], StructuredModel): | ||
| # CASE 2: Handle list fields. | ||
| # Note, if it's a primitive list, null/empty cases already handled in STEP 3. | ||
| elif is_list_field: |
There was a problem hiding this comment.
low-probability thing: this now routes on the static annotation where dev also required isinstance(gt_val, list) and isinstance(pred_val, list) before delegating. a non-list runtime value on a list-annotated field (reachable via model_construct or direct assignment, normal validation blocks it) now reaches compare_struct_list_with_scores and crashes in hungarian matching with AttributeError: 'str' object has no attribute 'compare_recursive' (verified with a str on either side at head), where dev classified the same input as FD. a cheap isinstance guard before this delegation would restore the graceful failure mode.
|
|
||
|
|
||
|
|
||
| class TestWeightedOverallScoreParity: |
There was a problem hiding this comment.
minor: the restore landed as a duplicate, TestWeightedOverallScoreParity is now defined twice in this file (here and at :965, which is the copy that came in via the dev merge). the bodies are identical so pytest just collects the second definition and nothing is lost today, but the first copy is dead code and it'll bite when someone edits one and not the other. worth dropping one.
|
|
||
| print("Running comprehensive aggregate tests...") | ||
|
|
||
| try: |
There was a problem hiding this comment.
nit on the __main__ block: the try/except prints the failure and exits 0, so running the file directly looks green even when a test fails. since pytest is the real runner i'd drop the block entirely. also two stale comments upstream contradict their assertions: :263 says "5 true positives... 1 true negative" vs the asserted tp=4/tn=2 at :264-267, and :684-685 says "aggregate TN=2 (one per leaf...)" vs the asserted TN=1 at :710.
…on fix from #154) # Conflicts: # uv.lock
5 participants
Issue #, if available: #94
Description of changes:
Updated Hungarian Matching for Structured Lists
Updated
StructuredListComparatorDispatcher Logic Refactoring:
_is_structured_list_field)StructuredListComparatorImproved Test Cases for Aggregate Functionality:
TestWeightedOverallScoreParity(bulk-vs-per-doc parity coverage)Enhanced Bulk Evaluator with Aggregate Support:
Addedinclude_aggregatesparameter toBulkStructuredModelEvaluatorNewrecall_with_fdparameter for flexible recall calculationsAggregateConfusionMatrixAccumulatoris included in the default accumulators list (added by @sromoam). No extra configuration needed.recall_with_fdis a constructor arg on the accumulator (e.g.,AggregateConfusionMatrixAccumulator(recall_with_fd=True)), not on the evaluator.Dead Code Removal:
_handle_hierarchical_field,_handle_non_hierarchical_field,_add_derived_metrics_recursively,_should_use_hierarchical_structure, leaf-seeding helpers) and stale references incomparison_dispatcher.pyandarchitecture.md.Update documentation accordingly: docs/docs/Advanced/aggregate-metrics.md; docs/docs/Advanced/threshold-gated-evaluation.md
Assumption: For aggregate metrics, all matched items (regardless of the similarity score) are considered. Zero-similarity pairs are excluded (treated as unmatched, not matched).
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.