Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

DICOM redactor improvement: Enabling compatibility with compressed images #1105

Merged
merged 64 commits into from
Jul 13, 2023
Merged

DICOM redactor improvement: Enabling compatibility with compressed images #1105

merged 64 commits into from
Jul 13, 2023

Conversation

niwilso
Copy link
Collaborator

@niwilso niwilso commented Jul 3, 2023 •
edited
Loading

Change Description

The current DICOM redactor is not currently compatible with compressed images. This PR introduces changes to enable use with compressed images.

Issue reference

This PR fixes issue #1040

Checklist

  • I have reviewed the contribution guidelines
  • I have signed the CLA (if required)
  • My code includes unit tests
  • All unit tests and lint checks pass locally
  • My PR contains documentation updates / additions if required

@niwilso niwilso marked this pull request as ready for review July 3, 2023 21:09
@niwilso niwilso requested a review from a team as a code owner July 3, 2023 21:09
@niwilso niwilso requested review from omri374, navalev and SharonHart July 3, 2023 21:10
@niwilso niwilso changed the title DICOM redactor improvement: enable compatibility with compressed images DICOM redactor improvement: Enabling compatibility with compressed images Jul 3, 2023
@niwilso
Copy link
Collaborator Author

niwilso commented Jul 3, 2023

Build pipeline is hanging when running unit tests for the image redactor module for Python versions 3.9 and 3.10. Will look into this on Wednesday (unless anyone has any insight).

@niwilso niwilso requested a review from ayabel July 5, 2023 13:48
omri374
omri374 reviewed Jul 5, 2023
View reviewed changes
NOTICE Show resolved Hide resolved
presidio-image-redactor/Pipfile.lock Outdated Show resolved Hide resolved
@niwilso niwilso mentioned this pull request Jul 5, 2023
Merged
5 tasks
ayabel
ayabel previously approved these changes Jul 6, 2023
View reviewed changes
omri374
omri374 previously approved these changes Jul 6, 2023
View reviewed changes
@niwilso
Copy link
Collaborator Author

niwilso commented Jul 6, 2023

Note, I will add a few more lines of code once PR #1109 is merged in. I was originally planning on merging this compression bug fix PR first and then building upon those changes in PR 1109, but have decided to reverse the order of modifying the code to accommodate the delay from the pipfile.lock issue.

@SharonHart
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

No commit pushedDate could be found for PR 1105 in repo microsoft/presidio

niwilso and others added 26 commits July 11, 2023 08:36
@niwilso
Commenting out assertions for test_verify_correctly
1393bd3
@niwilso
Comment out test_verify_correctly assertions and full test_eval_dicom…
3b741df 
…_correctly test
@niwilso
Adding assertions back into test_verify_correctly
236d1f1
@niwilso
Commenting out last assertion in test_verify_correctly
2358ebb
@niwilso
Reformatting to avoid use of all in assert
4d3bb2b
@niwilso
Using frozen set comparison for final assert in test_verify_correctly
e6402b3
@niwilso
Removing final assertion from test_verify_correctly while keeping tes…
80d5241 
…t_eval_dicom_correctly in
@niwilso
Commenting out act and assert in test_verify_correctly while keeping …
6478092 
…test_eval_dicom_correctly in
@niwilso
Using module-level mock variables and moving act call to less lines i…
3374a08 
…n test_verify_correctly
@niwilso
Removing all assertions but keeping other sections for both tests
3f0ed5b
@niwilso
Adding back in first assertions for each test
ee9dc19
@niwilso
Removing all assertions except fist image type assertion in verify test
ffb0223
@niwilso
Removing all assertions except image type assertion in test_eval_dico…
ca230c3 
…m_correctly
@niwilso
Remove image type assertions but keep all other assertions in
049f4ad
@niwilso
Remove image type assertions and all assertions for eval test
6319df1
@niwilso
Only keep final assertion in verify test
dc4a7b4
@niwilso
Only keep the second assertion in verify and nothing else
2ac2870
@niwilso
Only keep final assertion in eval test
9353bbe
@niwilso
Enable all assertions and move helper methods above test methods
08a9d2d
@niwilso
Simplifying assertions
15c69d7
@niwilso
Removing unused code now that we have simplified assertions
42f4fa2
@niwilso
Reverting back but simplifying asserts another way
df3e247
@niwilso
Removing eval integration test since unit test for that covers functi…
36acd24 
…onality
@niwilso
Remvoing analyzer results assertion
4841959
@niwilso
Merge branch 'main' into niwilso/dicom/compressed-images
7b74e3d
@niwilso
Removing unused import
288ec9a
@niwilso niwilso merged commit 67833d5 into main Jul 13, 2023
@SharonHart SharonHart deleted the niwilso/dicom/compressed-images branch July 18, 2023 10:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ayabel ayabel ayabel approved these changes

@omri374 omri374 omri374 approved these changes

@navalev navalev Awaiting requested review from navalev

@SharonHart SharonHart Awaiting requested review from SharonHart

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@niwilso @SharonHart @omri374 @ayabel