Repository navigation
Conversation
Use complete expected parametric-map outputs for emitted metadata defaults, make the segmentation BodyPartExamined default explicit, and supply a deterministic tracking UID for the SR roundtrip. Preserve strict key and list comparison without new ignored fields. Validated 16 comparator/CLI tests, the three modified input schemas, and captured-output comparisons with negative controls. Native roundtrip CI remains required. Signed-off-by: Jeff <138259519+jdotc1@users.noreply.github.com>
Give the Intel macOS superbuild 90 minutes while retaining the 45-minute ARM limit. The uncached Intel build exhausted the previous limit before running tests. Assign the existing build step its referenced id so successful builds can save the dependency cache. Validated with actionlint 1.7.12, YAML structure comparison, and git diff --check. Tests and comparator strictness are unchanged. Signed-off-by: Jeff <138259519+jdotc1@users.noreply.github.com>
There was a problem hiding this comment.
🟢 Approval recommended
No unresolved issues were identified, and the reported current-head validation passed across all five platform configurations.
No open issues 🎉
What changed in this PR
Fixes strict JSON comparison to reject unexpected content, addressing #364 while preserving recursive containment behavior.
Changes:
- Checks dictionary key sets and list lengths, propagates containment mode, and forwards ignored keys correctly.
- Adds utility regressions to CTest and aligns native round-trip fixtures with complete output.
- Extends the Intel macOS timeout and identifies the build step for dependency-cache saving.
| File | Description |
|---|---|
util/test_jsoncompare.py |
Adds comparator and command-line regression coverage. |
util/jsoncompare.py |
Corrects strict comparison, recursive containment, and ignored-key forwarding. |
doc/examples/sr-tid1500-ct-liver-example.json |
Adds a deterministic tracking UID. |
doc/examples/seg-example.json |
Includes the emitted body-part field. |
doc/examples/seg-example_multiple_segments.json |
Includes the emitted body-part field for multiple segments. |
CMakeLists.txt |
Registers utility tests with CTest. |
apps/paramaps/Testing/Expected/pm-example.json |
Adds complete integer parametric-map metadata expectations. |
apps/paramaps/Testing/Expected/pm-example-float.json |
Adds complete floating-point parametric-map metadata expectations. |
apps/paramaps/Testing/CMakeLists.txt |
Uses the complete metadata fixtures for round-trip tests. |
.github/workflows/cmake-macos.yml |
Adjusts the Intel timeout and adds the build-step identifier. |
Review effort: Balanced
There was a problem hiding this comment.
Thanks for this PR, @jdotc1. The json_are_same argument fix and the IndexError fix both look correct, and tightening strict comparisons is a good change.
Disclosure: this review, including all of the inline comments, was produced with Claude (Anthropic's AI assistant, via Claude Code) and posted by @fedorov.
None of the findings breaks a test. The ones that matter most:
- A fixed
TrackingUniqueIdentifieris now in the public SR example, so users who copy it will reuse one UID. - Strict-mode key mismatches no longer say which keys differ.
- The paramap round trip now depends on hand-maintained copies under
Testing/Expected/.
The rest are smaller cleanups: duplicated fixtures, __pycache__ written to the source tree, dead code, and a redundant branch.
Would you like us to apply these fixes and push them to this branch, or would you prefer to address them yourself? Either is fine; just let us know.
🤖 Generated with Claude Code
| "Measurements": [ | ||
| { | ||
| "TrackingIdentifier": "Measurements group 1", | ||
| "TrackingUniqueIdentifier": "2.25.120749835977160595394970525196798946045", |
There was a problem hiding this comment.
This puts a fixed TrackingUniqueIdentifier into the public example so the strict round trip stays stable. tid1500writer.cxx (lines 194-199) only generates a fresh UID when the input JSON doesn't have one. Anyone who copies this documented example will produce SR objects that all share the same tracking UID (2.25.120749835977160595394970525196798946045), even across different patients and findings. That breaks the uniqueness that longitudinal tracking depends on.
Suggestion: keep the example as it was and add TrackingUniqueIdentifier to the ignore list of the tid1500_meta_roundtrip comparejson call in apps/sr/Testing/CMakeLists.txt.
🤖 Comment generated by Claude (Claude Code), posted on behalf of @fedorov.
There was a problem hiding this comment.
Removed the fixed tracking UID in 6f8ee01 so the writer generates it, and excluded only TrackingUniqueIdentifier from the SR round-trip comparison. Changed tracking identifiers and unrelated extra fields still fail comparison.
| ignored = set(ignore_value_of_keys) | ||
| if set(expected) - ignored != set(actual) - ignored: | ||
| return False, Stack().append( | ||
| StackItem('Different keys', expected, actual)) |
There was a problem hiding this comment.
When the key sets differ, this reports only Different keys plus both full dicts. The removed code listed exactly which keys were missing and which were ignored. For full SR or SEG outputs (hundreds of lines) with one extra key, CI will print both JSON trees with nothing pointing at the culprit.
Suggestion: include sorted((set(expected) - ignored) - set(actual)) and sorted((set(actual) - ignored) - set(expected)) in the message.
🤖 Comment generated by Claude (Claude Code), posted on behalf of @fedorov.
There was a problem hiding this comment.
The diagnostic now lists sorted missing and extra keys after excluding ignored keys. Direct, nested, and CLI regression cases pass in 6f8ee01 and fail on the previous implementation.
| return expected == actual, Stack() | ||
|
|
||
| if not ignore_missing_keys and len(expected) > len(actual): | ||
| all_missing_keys = get_missing_keys(expected, actual) |
There was a problem hiding this comment.
get_missing_keys (line 140) loses its only caller with this removal, so it's now dead code. Either delete it, or reuse it for the missing and extra key diagnostic suggested on line 124.
🤖 Comment generated by Claude (Claude Code), posted on behalf of @fedorov.
There was a problem hiding this comment.
Removed the unused get_missing_keys helper in 6f8ee01. The diagnostic uses the filtered key sets directly.
| if isinstance(expected, dict): | ||
| return _is_dict_same(expected, actual, ignore_value_of_keys) | ||
| if not ignore_missing_keys: | ||
| ignored = set(ignore_value_of_keys) |
There was a problem hiding this comment.
Minor: set(ignore_value_of_keys) is rebuilt at every nested dict, which adds up on large SR and SEG trees. Normalizing ignore_value_of_keys to a set once in are_same / contains and passing it down would avoid that.
Also, with this key-set check in place, the if key not in actual: branch in _is_dict_same (line 57) can no longer fire in strict mode. It now only matters for contains mode, which may be worth a comment.
🤖 Comment generated by Claude (Claude Code), posted on behalf of @fedorov.
There was a problem hiding this comment.
Ignored keys are now normalized once in are_same and contains and reused recursively. A one-shot iterable regression covers nested dictionaries and lists in both modes. I also documented the containment-only missing-key branch.
| MODULE_NAME ${MODULE_NAME} | ||
| COMMAND ${Python3_EXECUTABLE} ${CMAKE_SOURCE_DIR}/util/comparejson.py | ||
| ${CMAKE_SOURCE_DIR}/doc/examples/pm-example.json | ||
| ${CMAKE_CURRENT_SOURCE_DIR}/Expected/pm-example.json |
There was a problem hiding this comment.
The paramap round trip now compares against hand-maintained copies in Testing/Expected/ instead of doc/examples/pm-example.json. The copy repeats every input field and adds the output-only keys, so it can drift from the input that is actually converted. The SEG fixes in this PR edit the doc examples in place instead, which leaves two different fixture strategies.
Suggestion: compare against the doc example and add the output-only keys (DerivationDescription, DerivedPixelContrast, ...) to the ignore list. Alternatively, generate Expected/ from the doc example.
🤖 Comment generated by Claude (Claude Code), posted on behalf of @fedorov.
There was a problem hiding this comment.
Both paramap round trips now use the original doc/examples inputs and ignore only DerivationDescription and DerivedPixelContrast, alongside the existing @schema exclusion. Positive comparisons pass; missing input fields and unrelated extra keys still fail.
| @@ -0,0 +1,39 @@ | |||
| { | |||
There was a problem hiding this comment.
This file and Expected/pm-example.json are 39-line near-duplicates that differ only in MeasurementUnitsCode (m2/s vs um2/s). Any future change to the paramap output schema has to be applied by hand to both copies and to the doc examples. If the previous comment's approach works, both files could go away.
🤖 Comment generated by Claude (Claude Code), posted on behalf of @fedorov.
There was a problem hiding this comment.
Removed both duplicate Testing/Expected/pm-example*.json files in 6f8ee01. Each round trip now compares against its original public example.
| add_subdirectory("libsrc") | ||
|
|
||
| if(BUILD_TESTING) | ||
| add_test(NAME dcmqi_jsoncompare |
There was a problem hiding this comment.
unittest discover -s <source>/util writes util/__pycache__/*.pyc into the source tree, so an out-of-source ctest run leaves the checkout dirty, which is a problem for read-only or shared source trees. Adding -B to the Python invocation (or setting PYTHONDONTWRITEBYTECODE=1 via set_tests_properties(... ENVIRONMENT ...)) avoids it.
🤖 Comment generated by Claude (Claude Code), posted on behalf of @fedorov.
There was a problem hiding this comment.
Added -B to the utility CTest, all nine metadata-comparison commands, and CLI test subprocesses. The out-of-source harness ran all ten Python CTests with PYTHONDONTWRITEBYTECODE unset and created no source-tree .pyc files.
There was a problem hiding this comment.
Hi @fedorov edorov, thank you again for the thorough review. I've addressed all seven points in commit 6f8ee01 and responded to each comment inline.
When you have a chance, would you mind taking another look? I'd be happy to address anything that remains.
Thanks again!
Report missing and extra keys, normalize ignored keys once, and cover nested iterable ignores and CLI diagnostics. Compare paramap outputs to the public examples with only explicit output fields ignored, restore generated SR tracking UIDs, and disable bytecode in Python test processes. Signed-off-by: Jeff <138259519+jdotc1@users.noreply.github.com>
|
|
Addressed all seven review points in 6f8ee01 and replied inline. The 18 utility tests and ten-command Python CTest harness pass, including strict negative controls and the bytecode check. The new revision has also passed 94/94 native CTests on macOS ARM64; the remaining platform builds are running. |
|
@claude review |
There was a problem hiding this comment.
If your organization's extra usage balance is empty, an organization admin can add extra usage credits at claude.ai/admin-settings/usage. If its monthly spend limit was reached, an admin can raise it on the same page. If neither applies, contact Anthropic support.
Once extra usage is available, someone with write access to this repository can comment @claude review on this pull request to trigger a review.
Strict JSON comparison previously accepted unexpected keys and extra list values, and a shorter list with duplicates could raise
IndexErrorinstead of reporting a mismatch.Compare filtered dictionary key sets and ordered-list lengths in strict mode, propagate containment mode recursively, forward ignored keys in
json_are_same, and register the utility regressions with CTest. Strict key mismatches report sorted missing and extra keys, and ignored keys are normalized once before recursion.Round-trip comparisons use the public examples. Paramap comparisons exclude the two output-only fields (
DerivationDescriptionandDerivedPixelContrast). The SR writer generates its tracking UID, which is excluded from the SR round-trip comparison. All other strict comparisons remain enforced. Python CTests and their CLI subprocesses disable bytecode generation.The Intel macOS cold superbuild also exhausted the existing 45-minute job limit before reaching tests. Allow 90 minutes for Intel while retaining 45 minutes for ARM, and identify the build step so the existing successful-build dependency-cache save condition can work.
Addresses #364.
Testing
6f8ee01: 18 standard-library utility tests pass, including six CLI cases. Seven new regression subcases fail before the review fixes..pycfiles are created withPYTHONDONTWRITEBYTECODEunset.git diff --checkpasses. Native converters were not rebuilt locally for this revision; fresh hosted builds are running.492bc409: 94/94 CTests passed on each of Linux x86_64 and ARM64, macOS Intel and ARM64, and Windows. These results precede the review fixes.Compatibility
Strict comparison rejects additional dictionary keys and list values. Existing heterogeneous-sort ordering is unchanged.