Bug Fixes and Unit Tests for BLANK & REPLACE Actions & Sequences #242

wetzelj · 2022-11-30T20:31:16Z

Description

Updated BLANK action to function on all VRs.
Added unit tests to ensure BLANK functions on all newly added VRs.
Added unit tests to validate multiple actions on a single field. Added documentation which describes the current interactions.
Corrected issue with REPLACE on numeric VRs (and URI as well).
Added unit tests to validate REPLACE on all (applicable?) VRs. Currently SQ, and the O* VRs are excluded.
Corrected issue preventing actions from executing on fields within sequences.

Related issues:

Checklist

I have commented my code, particularly in hard-to-understand areas
My changes generate no new warnings
My code follows the style guidelines of this project

Open questions

For the interaction unit tests and documentation, I simply test and document what the library currently does as of v0.3.1. I would appreciate a closer review of this. As I was building these tests, some of the interactions were not what I expected... but that may just be my mind working a little differently than others! :)

1. Expanded BLANK Action to other VRs. 2. Added unit tests for validating action interaction. 3. Added table of action interactions to help docs.

wetzelj · 2022-11-30T20:37:41Z

The testing is failing due to the changes that were merged into deid-data. The CI testing is still using the old image.

vsoch · 2022-11-30T21:04:17Z

Old image == old release?

vsoch · 2022-11-30T21:05:00Z

@wetzelj if you can do a PR to bump the version / changelog over there, I can make a new release!

wetzelj · 2022-11-30T21:05:52Z

Yeah... sorry about that. I just realized I didn't bump the version of deid-data. Will do it now.

Correcting bad test method names.

docs/_docs/user-docs/recipe-headers.md

vsoch · 2022-12-01T00:18:42Z

okay sorry for delay - went into meetings and just coming out! I just merged/released deid-data and did a re-run of the test.

deid/dicom/parser.py

Reverted expand_field_expression to always return desired as a Tag. Instead convert to DataElement in blank_field.

Updating BLANK test coverage to all VRs.

Updating unit tests failing due to field count changes in test image.

Restructured blank_action unit tests.

vsoch · 2022-12-02T17:27:28Z

Woot! passing now, and these tests are fantastic! @wetzelj when you are ready for merge, it shall be so.

wetzelj · 2022-12-02T18:02:53Z

Thanks! I'm working on a fix for #244 as well that I'd like to get included in this PR (along with some new tests for REPLACE which mirror what I did for BLANK).

vsoch · 2022-12-02T19:24:49Z

@wetzelj you're amazing!!

deid/utils/actions.py

wetzelj · 2022-12-02T21:16:59Z

@vsoch - I think I'm set on what I'd like to get in this PR, but am interested in your feedback if there's anything that you'd like done differently.

.github/workflows/main.yaml

deid/utils/actions.py

Removing deid-data pin.

Co-authored-by: Vanessasaurus <814322+vsoch@users.noreply.github.com>

Committing review suggestions.

wetzelj · 2022-12-06T22:27:50Z

Adding #243 to this PR as well. Please hold off merging. New commits will arrive tomorrow.

Corrected issue pydicom#243 which prevented tags within sequences from being acted upon. Added unit tests for sequence processing.

Updated test_replace_identifiers to target ctbrain1.dcm by name.

Updated test_user_provided_func to target ctbrain1.dcm

wetzelj · 2022-12-07T14:20:11Z

deid/dicom/parser.py

@@ -160,8 +160,9 @@ def get_nested_field(self, field, return_parent=False):
            # Otherwise it's an index into a sequence
            else:



Here's an explanation of the change below:

If we fall into the else, the uid is a sequence. We don't want to check for it's presence in a dictionary, but instead want to check to ensure that the uid (index) is within the bounds of the array of items in the SQ DataElements value which is a Sequence - an iterable. If the index is outside the bounds we return None as the parent. If the index is within the bounds of the array, we want to return the DataElement at that index (the specific occurrence of the
sequence element) for update.

wetzelj · 2022-12-07T14:25:22Z

deid/tests/test_replace_identifiers.py

@@ -118,16 +118,12 @@ def test_replace_with_constant(self):
        ]
        recipe = create_recipe(actions)



Explanation of the change below:

After implementing the change for sequence processing, this unit test was failing with an index out of bounds exception. This exception was occurring because the DicomParser object was applying the stock deid.dicom recipe which was causing the Accession Number field to be removed. I did not dig into why the sequence change affected this unittest in this manner, but since it was only impacting the check for getting the "before" values and all other unit tests passed, I didn't feel that it warranted further investigation. Instead I just updated this to red the file and directly retrieve the "before" values

wetzelj · 2022-12-07T14:43:59Z

@vsoch - This wraps up what I'm intending (plus a little more) for this PR. Once you're good with the inclusions, please go ahead with the merge at your convenience.

vsoch · 2022-12-07T15:03:50Z

Woohoo! Well done. https://pypi.org/project/deid/0.3.2/

wetzelj · 2022-12-07T15:04:55Z

Woohoo! Well done. https://pypi.org/project/deid/0.3.2/

Thank you! Speedy merge as always! :)

vsoch · 2022-12-07T15:05:53Z

I just happen to be up since 6am debugging registry authentication for another OSS project - trying to squeeze in before the workday. Hashtag, maintainer life! 😆

wetzelj added 4 commits November 30, 2022 14:48

Expand BLANK Action to other VRs

b87e857

1. Expanded BLANK Action to other VRs. 2. Added unit tests for validating action interaction. 3. Added table of action interactions to help docs.

Update recipe-headers.md

9e5c39b

Update recipe-headers.md

6365449

Bumping version and adding changelog

62f1447

Fixed Linting Errors

cdf099d

Update test_action_interaction.py

80fec63

Correcting bad test method names.

wetzelj commented Nov 30, 2022

View reviewed changes

docs/_docs/user-docs/recipe-headers.md Show resolved Hide resolved

Update deid-data install version

6e7d299

vsoch reviewed Dec 1, 2022

View reviewed changes

deid/dicom/parser.py Outdated Show resolved Hide resolved

wetzelj added 3 commits December 1, 2022 10:12

Update parser.py

320bfd5

Reverted expand_field_expression to always return desired as a Tag. Instead convert to DataElement in blank_field.

Expanding blank to all VRs

2ebe589

Updating BLANK test coverage to all VRs.

Updating field counts in unit tests.

b722326

Updating unit tests failing due to field count changes in test image.

This was referenced Dec 2, 2022

Tags within sequences (nested fields) are not affected by header actions #243

Closed

REPLACE action fails on numeric VR #244

Closed

Update test_blank_action.py

68832dd

Restructured blank_action unit tests.

wetzelj added 2 commits December 2, 2022 15:57

Fix for pydicom#244 - Replace fails for numeric VR

1dcae46

Fixing isort issue.

94a4eaf

wetzelj changed the title ~~Expand BLANK Action to Additional VRs~~ Bug Fixes and Unit Tests for BLANK & REPLACE Dec 2, 2022

Update CHANGELOG.md

b81a180

wetzelj commented Dec 2, 2022

View reviewed changes

deid/utils/actions.py Show resolved Hide resolved

vsoch reviewed Dec 2, 2022

View reviewed changes

.github/workflows/main.yaml Outdated Show resolved Hide resolved

deid/utils/actions.py Outdated Show resolved Hide resolved

deid/utils/actions.py Show resolved Hide resolved

deid/utils/actions.py Outdated Show resolved Hide resolved

deid/utils/actions.py Outdated Show resolved Hide resolved

wetzelj and others added 3 commits December 2, 2022 16:38

Update main.yaml

12f3518

Removing deid-data pin.

Update deid/utils/actions.py

8dcb703

Co-authored-by: Vanessasaurus <814322+vsoch@users.noreply.github.com>

Update actions.py

86096a8

Committing review suggestions.

Header Sequence Updates

8af88d4

Corrected issue pydicom#243 which prevented tags within sequences from being acted upon. Added unit tests for sequence processing.

wetzelj changed the title ~~Bug Fixes and Unit Tests for BLANK & REPLACE~~ Bug Fixes and Unit Tests for BLANK & REPLACE Actions & Sequences Dec 7, 2022

wetzelj added 3 commits December 7, 2022 09:02

Update CHANGELOG.md

88123e3

Update test_replace_identifiers.py

e543abd

Updated test_replace_identifiers to target ctbrain1.dcm by name.

Update test_dicom_funcs.py

a1f21a5

Updated test_user_provided_func to target ctbrain1.dcm

wetzelj commented Dec 7, 2022

View reviewed changes

vsoch approved these changes Dec 7, 2022

View reviewed changes

vsoch merged commit bd3994f into pydicom:master Dec 7, 2022

vsoch mentioned this pull request Dec 7, 2022

BLANK Action only applicable to US and SS VRs #241

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug Fixes and Unit Tests for BLANK & REPLACE Actions & Sequences #242

Bug Fixes and Unit Tests for BLANK & REPLACE Actions & Sequences #242

wetzelj commented Nov 30, 2022 •

edited

Loading

wetzelj commented Nov 30, 2022

vsoch commented Nov 30, 2022

vsoch commented Nov 30, 2022

wetzelj commented Nov 30, 2022

vsoch commented Dec 1, 2022

vsoch commented Dec 2, 2022

wetzelj commented Dec 2, 2022

vsoch commented Dec 2, 2022

wetzelj commented Dec 2, 2022

wetzelj commented Dec 6, 2022

wetzelj Dec 7, 2022

wetzelj Dec 7, 2022

wetzelj commented Dec 7, 2022

vsoch commented Dec 7, 2022

wetzelj commented Dec 7, 2022

vsoch commented Dec 7, 2022

		@@ -160,8 +160,9 @@ def get_nested_field(self, field, return_parent=False):
		# Otherwise it's an index into a sequence
		else:

		@@ -118,16 +118,12 @@ def test_replace_with_constant(self):
		]
		recipe = create_recipe(actions)

Bug Fixes and Unit Tests for BLANK & REPLACE Actions & Sequences #242

Bug Fixes and Unit Tests for BLANK & REPLACE Actions & Sequences #242

Conversation

wetzelj commented Nov 30, 2022 • edited Loading

Description

Checklist

Open questions

wetzelj commented Nov 30, 2022

vsoch commented Nov 30, 2022

vsoch commented Nov 30, 2022

wetzelj commented Nov 30, 2022

vsoch commented Dec 1, 2022

vsoch commented Dec 2, 2022

wetzelj commented Dec 2, 2022

vsoch commented Dec 2, 2022

wetzelj commented Dec 2, 2022

wetzelj commented Dec 6, 2022

wetzelj Dec 7, 2022

Choose a reason for hiding this comment

wetzelj Dec 7, 2022

Choose a reason for hiding this comment

wetzelj commented Dec 7, 2022

vsoch commented Dec 7, 2022

wetzelj commented Dec 7, 2022

vsoch commented Dec 7, 2022

wetzelj commented Nov 30, 2022 •

edited

Loading