Skip to content

BUG: fix infer_contrast for missing ContrastBolusAgent tag#85

Open
Ivan-E-Johnson wants to merge 3 commits into
mainfrom
fix-infer-contrastbolusagent
Open

BUG: fix infer_contrast for missing ContrastBolusAgent tag#85
Ivan-E-Johnson wants to merge 3 commits into
mainfrom
fix-infer-contrastbolusagent

Conversation

@Ivan-E-Johnson
Copy link
Copy Markdown
Collaborator

@Ivan-E-Johnson Ivan-E-Johnson commented Apr 3, 2026

Summary

  • Fixes a bug where DICOM volumes missing the ContrastBolusAgent tag were incorrectly classified as having contrast
  • Moves the fix to the correct layer: adds "-12345" (the sentinel for missing optional fields) to the no_contrast_list in get_coded_dictionary_elements() (utility_functions.py) so the value is normalized to "None" at the sanitization layer rather than patched downstream in infer_contrast()
  • Reverts the downstream band-aid in infer_contrast() since it is no longer needed
  • Regenerated the integration test baseline output.json to match the corrected output

Test plan

  • test_dcm_series_no_contrast — verifies series with no contrast tag returns False
  • test_dcm_series_has_contrast — verifies series with a real contrast agent still returns True
  • test_classify_study_json — integration test comparing full JSON output against regenerated baseline
  • Full suite: python3 -Werror::FutureWarning -m pytest tests — 81 passed, 3 skipped

Notes

Builds on the original work from PR #84 by @ChanceSauley, including the new no_contrastbolusagent_tag.dcm test file and improved assertion message in test_dcm_series_no_contrast.

🤖 Generated with Claude Code

ChanceSauley and others added 3 commits January 16, 2026 13:23
This function worked (and validated by a test) when ContrastBolusAgent tag existed and had the value of "NONE".

However, if the tag never existed in the source it's value would be set to -12345 during sanitation. This would cause all series without the tag present to be infered as having contrast.

Add a check for the default value and a test to validate functionality. The test file is a copy of the existing test file with the ContrastBolusAgent tag removed and UIDs changed.

The integration test reference file was changed to reflect the correct result.
…sanitization layer

The previous fix checked for the sentinel value "-12345" in
infer_contrast(), but this left the raw sentinel flowing through the
rest of the system. Instead, add "-12345" to the no_contrast_list in
get_coded_dictionary_elements() so missing tags are normalized to
"None" before reaching any downstream code. Regenerated the
integration test baseline to match the corrected output.
@Ivan-E-Johnson Ivan-E-Johnson force-pushed the fix-infer-contrastbolusagent branch from a26a83d to 5e7f5d1 Compare April 3, 2026 17:29
@Ivan-E-Johnson Ivan-E-Johnson requested a review from mbrzus April 3, 2026 17:33
@Ivan-E-Johnson Ivan-E-Johnson self-assigned this Apr 3, 2026
@ChanceSauley
Copy link
Copy Markdown

ChanceSauley commented Apr 3, 2026

I'm not actually sure why this is passing Actions now. Nothing should have changed to affect the failed test in my PR, and it is still failing locally for me. Would be interested to see if anyone can replicate it if they have a Ubuntu 24 machine/VM handy.

image

@mbrzus
Copy link
Copy Markdown
Collaborator

mbrzus commented Apr 8, 2026

I'm not actually sure why this is passing Actions now. Nothing should have changed to affect the failed test in my PR, and it is still failing locally for me. Would be interested to see if anyone can replicate it if they have a Ubuntu 24 machine/VM handy.

Thanks @ChanceSauley we will investigate and run the testing on Ubuntu this Friday.

@CavRiley
Copy link
Copy Markdown
Collaborator

@Ivan-E-Johnson, on which OS was this generated?

Based on @ChanceSauley most recent PR (#86) which is failing in a similar I area I believe, we may need to update this test to work across platforms.

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.

4 participants