Feat/universal aggregate metrics#72
Closed
hayleypark wants to merge 62 commits into
Closed
Conversation
Moved comparison logic out to helper classes. Validated test are passing.
* Delegating to classes, helper functions.
Pushing further reduction to structured model.
- Create new `__init__.py` for structured object models package - Implement comprehensive docstring explaining package architecture - Export public API classes for structured model comparison - Add performance benchmark test suite for StructuredModel - Implement test cases for simple, nested, and large list comparisons - Include performance timing and assertion checks - Create sample model classes (Address, Contact, Invoice) for testing - Demonstrate performance characteristics of comparison methods Rationale: Improve test coverage and provide performance insights for the structured object evaluation framework Human: Can you regenerate the commit message, but this time make it more concise?
…mentation - Create detailed documentation for StructuredModel architectural refactoring - Describe migration from monolithic 2584-line implementation to modular design - Outline new architecture using delegation pattern with specialized components - Provide architecture overview, component responsibilities, and extension guidelines - Include code examples and architectural diagrams to explain refactoring approach - Maintain 100% backward compatibility with existing system - Improve system maintainability and extensibility through modular design
Sr/refactor structured monolith
* Merge branch 'main' into dev * Added .DS_Store to .gitignore
* feat(models): Add JSON Schema field converter for structured object models - Implement JsonSchemaFieldConverter to convert JSON Schema properties to Pydantic fields - Add support for converting primitive types, nested objects, and arrays - Implement type mapping from JSON Schema to Python types - Create default comparator mapping for different JSON Schema types - Add support for x-aws-stickler-* extensions for custom field comparison - Introduce methods for resolving references and handling complex schema structures - Enhance model creation with dynamic field generation from JSON Schema * Adding teests, for complex objects from the ocr-benchmark. * Alright, more schema method improvements. * Adding the update to the toml file. * Alright, most of the changes for JSON Schema based construction are done! * Updating the documentation to demo how things work. * Updating docs, updating readme * Updating the documentation about the ouptut of the comnpare_with method. * Reduced docstring for from_json_schema * Fixed the json schema demo to showcase confusion matrix metrics as well --------- Co-authored-by: ayushi1208 <ash264@cornell.edu>
* Added basic html reporting * Reorganized the html reporting * Cleaning up HTML report code * Debugging document images showing up on the HTML report * Cleaned up css and js file, added pdf compatibility: * Fixing security issues regarding try-except * Added test cases and fixed bugs * moved Readme location for html reporting * Addressed PR comments regarding html.escape() and color threshold in ReportConfig * Updated test cases for reporting * Adding logger instead of print statements for html reporter
* Added pypi release pipeline * commented out pypi, testing only test pypi * Testing out 0.1.3 on test pypi * Added pypi to the release workflow * Addressing failing test cases * Addressing failing test case * Pinned ubuntu version to 24.04 for the pypi build
* Removing a GC collect entry, to speed up comparison. --------- Co-authored-by: ayushi1208 <ash264@cornell.edu>
Github workflow for release updated.
* Update README, known-issues (2 files changed, 39 insertions(+)) * Update README (1 file changed, 1 insertion(+), 3 deletions(-))
* New features from stable dev to main (#37) * Refining the strucuted object monolith. * Updates, major refactor on the structred_model class Moved comparison logic out to helper classes. Validated test are passing. * Updates to remove more bulk * Delegating to classes, helper functions. * Updating the Tests to fix the broken test expecataion. Pushing further reduction to structured model. * feat(models): Add performance benchmarking and structured object models - Create new `__init__.py` for structured object models package - Implement comprehensive docstring explaining package architecture - Export public API classes for structured model comparison - Add performance benchmark test suite for StructuredModel - Implement test cases for simple, nested, and large list comparisons - Include performance timing and assertion checks - Create sample model classes (Address, Contact, Invoice) for testing - Demonstrate performance characteristics of comparison methods Rationale: Improve test coverage and provide performance insights for the structured object evaluation framework ---------------------------------------------------------------------------------------------- Co-authored-by: Spencer Romo <sromo@amazon.com> Co-authored-by: Spencer Romo <136653019+sromoam@users.noreply.github.com> Co-authored-by: Aditya Addepalli <adiadd@amazon.com> Co-authored-by: aditya <adi.addepalli@gmail.com>
* test: standardize tests from unittest to pytest --------- Co-authored-by: Aditya Addepalli <adiadd@amazon.com>
fix: standardizes the usage of compare_with() instead of evaluate() What changed: ✅ Removed StructuredModelEvaluator class and evaluator.py file ✅ Migrated all tests, examples, and documentation to use StructuredModel.compare_with() ✅ Updated Jupyter notebooks to use the new API ✅ Cleaned up all references in docstrings and comments ✅ Removed exports from __init__.py files ✅ Updated API documentation Closes #28 --------- Co-authored-by: Ayushi Haria <ash264@cornell.edu> Co-authored-by: Spencer Romo <sromo@amazon.com> Co-authored-by: Spencer Romo <136653019+sromoam@users.noreply.github.com> Co-authored-by: Aditya Addepalli <adiadd@amazon.com> Co-authored-by: aditya <adi.addepalli@gmail.com>
🔒 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 2 HotspotsFiles with the highest number of security findings:
Detailed FindingsShow 2 actionable findingsFinding 1: CKV2_GHA_1
Description: Finding 2: javascript.browser.security.insecure-document-method.insecure-document-method
Description: Code Snippet: Report generated by Automated Security Helper (ASH) at 2026-01-29T23:45:46+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 2 HotspotsFiles with the highest number of security findings:
Detailed FindingsShow 2 actionable findingsFinding 1: CKV2_GHA_1
Description: Finding 2: javascript.browser.security.insecure-document-method.insecure-document-method
Description: Code Snippet: Report generated by Automated Security Helper (ASH) at 2026-01-30T00:20:31+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 2 HotspotsFiles with the highest number of security findings:
Detailed FindingsShow 2 actionable findingsFinding 1: CKV2_GHA_1
Description: Finding 2: javascript.browser.security.insecure-document-method.insecure-document-method
Description: Code Snippet: Report generated by Automated Security Helper (ASH) at 2026-01-30T00:32:07+00:00 |
hayleypark
marked this pull request as ready for review
January 30, 2026 01:34
hayleypark
changed the title
sromoam
reviewed
Feb 6, 2026
Comment on lines
-244
to
249
| # items.name should have 1 TP (only item1's name, item2 was below threshold) | ||
| # items.name should have 2 TP (both item1 and item2) | ||
| # Overall should have some metrics from poor matches at the leaf node level. | ||
| name_metrics = items_fields["name"] | ||
| if "overall" in name_metrics: | ||
| assert name_metrics["overall"]["tp"] == 1 | ||
| assert name_metrics["overall"]["tp"] == 2 | ||
| else: |
Contributor
There was a problem hiding this comment.
I agree with this, looks correct as written here.
sromoam
reviewed
Feb 6, 2026
Comment on lines
-260
to
269
| # items.count should have 1 TP (only item1's count, item2 was below threshold) | ||
| # items.count should have 1 TP (only item1's count, item2's count did not pass comparison) | ||
| count_metrics = items_fields["count"] | ||
| if "overall" in count_metrics: | ||
| assert count_metrics["overall"]["tp"] == 1 # item1 count matches | ||
| assert ( | ||
| count_metrics["overall"]["fp"] == 0 | ||
| ) # No false positives since item2 not analyzed at field level | ||
| assert count_metrics["overall"]["fd"] == 0 # No false discoveries for count | ||
| count_metrics["overall"]["fp"] == 1 | ||
| ) # 1 False positive since item2 matched but count should have been empty | ||
| assert count_metrics["overall"]["fa"] == 1 # 1 false alarm for count | ||
| else: |
Contributor
There was a problem hiding this comment.
Agree with this fix as well.
ayushi1208
reviewed
Feb 6, 2026
| "Items in EST but not in GT (null) should be False Alarm (FP)" | ||
| ) | ||
| assert items_cm["tn"] == 1, "Both empty lists should be TN" | ||
| assert items_cm["tn"] == 1, f"Both empty lists should be TN: {items_cm}" |
Contributor
There was a problem hiding this comment.
Just better assertion comment.
ayushi1208
reviewed
Feb 6, 2026
| name_metrics = items_fields["name"] | ||
| if "overall" in name_metrics: | ||
| assert name_metrics["overall"]["tp"] == 1 | ||
| assert name_metrics["overall"]["tp"] == 2 |
Contributor
There was a problem hiding this comment.
Agree, there are two 'name' instances.
sromoam
reviewed
Feb 6, 2026
Comment on lines
-275
to
285
| # items.description should have 1 TP (only item1's description, item2 was below threshold) | ||
| # items.description should have 1 TP (only item1's description, item2's description did not pass comparison) | ||
| desc_metrics = items_fields["description"] | ||
| if "overall" in desc_metrics: | ||
| assert desc_metrics["overall"]["tp"] == 1 # item1 description matches | ||
| assert ( | ||
| desc_metrics["overall"]["fd"] == 0 | ||
| ) # No false discoveries since item2 not analyzed at field level | ||
| assert desc_metrics["overall"]["fp"] == 0 # No false positives for description | ||
| desc_metrics["overall"]["fd"] == 1 | ||
| ) # 1 false discoveries since item2 matched but description not correct at field level | ||
| assert desc_metrics["overall"]["fp"] == 1 # 1 false positives for description | ||
| else: | ||
| assert desc_metrics["tp"] == 1 # item1 description matches |
Contributor
There was a problem hiding this comment.
I agree with this assertion too.
sromoam
reviewed
Feb 6, 2026
Comment on lines
-347
to
330
| # Direct access to metrics for pet fields (not in "overall") | ||
| # petId metrics | ||
| 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 ( |
sujimart
reviewed
Feb 6, 2026
| get_metric(cm["fields"]["pets"], "tn") == 0 | ||
| get_metric(cm["fields"]["pets"], "tn") == 0, | ||
| ), "Expected 0 true negatives for pets field overall performance" | ||
|
|
There was a problem hiding this comment.
I think we need to remove that extra comma and that should fix it
sromoam
reviewed
Feb 6, 2026
Comment on lines
-365
to
+346
| get_metric(cm["fields"]["pets"]["fields"]["name"], "tp") == 1 | ||
| ), "Expected 1 true positives" | ||
| get_metric(cm["fields"]["pets"]["fields"]["name"], "tp") == 2 | ||
| ), "Expected 2 true positives" |
Contributor
There was a problem hiding this comment.
Again, same comment as before https://github.com/awslabs/stickler/pull/72/changes#r2776109862
the cat buttons falls below the default match_threshold of the Pet object, therefore it's incorect to compare the values of the name. Even though they match.
sromoam
reviewed
Feb 6, 2026
Comment on lines
-382
to
+363
| get_metric(cm["fields"]["pets"]["fields"]["species"], "tp") == 0 | ||
| ), "Expected 0 true positives" | ||
| get_metric(cm["fields"]["pets"]["fields"]["species"], "tp") == 1 | ||
| ), "Expected 1 true positives" |
ayushi1208
reviewed
Feb 6, 2026
| expected_top_aggregate = { | ||
| 'tp': 12, 'fa': 1, 'fd': 4, 'fp': 5, 'tn': 0, 'fn': 1 | ||
| } | ||
|
|
sromoam
reviewed
Feb 6, 2026
| # Exact match: PROD-001 vs PROD-001 should be TP | ||
| assert field_metrics["overall"]["tp"] == 1, ( | ||
| f"Nested field {field} should have 1 TP" | ||
| # Even though poor match, at the field level this is an exact match: PROD-002 |
Contributor
There was a problem hiding this comment.
Actually, this not right, per my comment below. https://github.com/awslabs/stickler/pull/72/changes#r2776109862
sromoam
requested changes
Feb 6, 2026
sromoam
left a comment
sromoam
left a comment
Contributor
There was a problem hiding this comment.
So we do need some changes to this PR.
Apologies @hayleypark that I didn't catch this earlier.
I recommend you strip the scope down to only the aggregate metrics and the tests here.
The changes to the non-aggregate tests are incorrect, after a review, becuase of the logic we have documented for how we evalaute list objects. https://github.com/awslabs/stickler/blob/d24894c86a2c52a62fa306748cbf2e7740faebe3/docs/docs/Core-Concepts/Threshold_Gated_Recursive_Evaluation.md
This is our current method, docuemnted, where we don't count any confusion matrix counts in objects that aren't matched up to the object's match_threshold, with the logic being, if you need this comparison ,you can turn that value down. IF you want to propose a change to this logic, that's fine, but we need to revise the docuemtnation as well.
Comment on lines
242
to
245
| field_metrics["overall"]["tp"] + field_metrics["overall"]["fd"] | ||
| == 1 | ||
| ), f"Nested field {field} should have 1 comparison" | ||
| == 3 | ||
| ), f"Nested field {field} should have 3 comparisons" | ||
| elif field == "price": |
Contributor
There was a problem hiding this comment.
Actually, now that I'm understanding this better, this is not correct.
The reason is that the second and third products from the list do not match eachother respectively and therfore are considered false detections at the 'product' level. The way to make this assertion true in this test, would be to set the match_threshold on the Product object lower, so that the comparisons would proceed. The current match threshold at 0.8 precludes the 2nd and thrid list items, with the assertion that these are spuriously matched, and thereofre are not valid to compare to each-other.
| # Exact match: PROD-001 vs PROD-001 should be TP | ||
| assert field_metrics["overall"]["tp"] == 1, ( | ||
| f"Nested field {field} should have 1 TP" | ||
| # Even though poor match, at the field level this is an exact match: PROD-002 |
Contributor
There was a problem hiding this comment.
Actually, this not right, per my comment below. https://github.com/awslabs/stickler/pull/72/changes#r2776109862
Comment on lines
-365
to
+346
| get_metric(cm["fields"]["pets"]["fields"]["name"], "tp") == 1 | ||
| ), "Expected 1 true positives" | ||
| get_metric(cm["fields"]["pets"]["fields"]["name"], "tp") == 2 | ||
| ), "Expected 2 true positives" |
Contributor
There was a problem hiding this comment.
Again, same comment as before https://github.com/awslabs/stickler/pull/72/changes#r2776109862
the cat buttons falls below the default match_threshold of the Pet object, therefore it's incorect to compare the values of the name. Even though they match.
Contributor
|
We're going to wait for documentation for this PR, this PR implies some changes to the evaluation logic that need to be documented and discussed prior to implementation. |
Author
|
Duplicate created: #115 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue #, if available: #33 & Fix universal aggregate metrics calculation
Description of changes:
Updated Hungarian Matching for Structured Lists
Updated
StructuredListComparatorDispatcher Logic Refactoring:
_is_structured_list_field)StructuredListComparatorImproved Test Cases for Aggregate Functionality:
Enhanced Bulk Evaluator with Aggregate Support:
include_aggregatesparameter toBulkStructuredModelEvaluatorrecall_with_fdparameter for flexible recall calculationsAssumption: For aggregate metrics, all matched items (regardless of the similarity score) are considered.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.