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

Updates to clean and detect #150

Merged
merged 4 commits into from
Sep 22, 2020
Merged

Updates to clean and detect #150

merged 4 commits into from
Sep 22, 2020

Conversation

vsoch
Copy link
Member

@vsoch vsoch commented Sep 17, 2020

Description

Opening this PR as start of work to refactor the detect and clean flow, changes include:

  • removing the ultrasound regions from the default deid.dicom and adding one specifically for ultrasound
  • parsing them into one list, in the order provided, so keepcoordinates (value of 1) and coordinates (0) are respected for the order the user wants
  • having the default value set to ones (indicating keep)
  • adding an ability to say coordinates all or keepcoordinates all in case the user wants to apply a change to the entire mask (e.g., for the ultrasound we would do coordinates all to blank them first before keeping the Ultrasound region.
  • parsing the dicom in clean.py to account for these changes, and also adding image dimension of 2.
  • adding documentation that explains this!

cc @wetzelj

Related issues: #149

Please include a summary of the change(s) and if relevant, any related issues above.

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

Questions that require more discussion or to be addressed in future development:

@vsoch
start of work to fix detect clean
1115ceb 
Signed-off-by: vsoch <vsochat@stanford.edu>
@vsoch
adding ultrasound
dcff62a 
Signed-off-by: vsoch <vsochat@stanford.edu>
@vsoch
bump version of black
24049ba 
Signed-off-by: vsoch <vsochat@stanford.edu>
@wetzelj
Copy link
Contributor

wetzelj commented Sep 21, 2020

@vsoch - Are you okay with having mini, unit-test specific recipe files added for testing clean? If so, would a new "resources" folder under tests be okay?

@vsoch
Copy link
Member Author

vsoch commented Sep 21, 2020

Definitely!

@wetzelj
Copy link
Contributor

wetzelj commented Sep 21, 2020

I have some unit tests written as well as some bug fixes. They're currently committed here, but I can port them to where ever you want if you are okay with the changes.

Just to point out a couple things:

  • Removed the looping on line 156 in clean.py.
  • Switched the creation of the output folders from a mkdir to makedirs so that the entire tree is created.
  • Removed the "cleaned-" prefix from the output header files. This simplifies the merging of the pixel output with header output.

@vsoch
Copy link
Member Author

vsoch commented Sep 21, 2020

Great! Let's do the PR against the branch here, then we can merge, make sure all the tests still work, and then merge again and release. Everything sounds good except for:

Removed the "cleaned-" prefix from the output header files. This simplifies the merging of the pixel output with header output.

The concern here is that the user would accidentally rewrite files - I think we should keep the cleaned- prefix. It's also a message that the files were absolutely generated - without the prefix it might not be clear. If you feel strongly that this isn't a good feature, then we could have some kind of input argument for an output prefix, with default to be cleaned- and for your use case setting it to an empty string.

@wetzelj
Copy link
Contributor

wetzelj commented Sep 22, 2020

Got it.

I don't have a strong preference on the "cleaned-" prefix. Let's just leave it as is.

@wetzelj
Pr150 - Bug Fixes and Tests (#151)
66dc2d0 
* Adding tests for clean_pixels
Added tests for clean_pixels, corrected bugs discovered from tests.
* Added pixel clean  prefix
Added tests for clean_pixels, corrected bugs discovered from tests.
* Update clean.py
Revered prefix to what it had been prior to suggested prefix removal.
* Update test_clean.py
Formatting.
* coordinate handling
Adding test to validate coordinates retrieved from fields. Add check in clean to ensure coordinates are in a list prior to iterating.
vsoch
vsoch commented Sep 22, 2020
View reviewed changes
Copy link
Member Author

@vsoch vsoch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wetzelj this looks good to me, ready for release?

@wetzelj
Copy link
Contributor

wetzelj commented Sep 22, 2020

Same here. I will be continuing to test. If I see anything else I'll open another issue.

@vsoch vsoch merged commit f53e455 into master Sep 22, 2020
@vsoch
Copy link
Member Author

vsoch commented Sep 22, 2020

I'll give you a day or so to test and report any quick issues, otherwise I'll release after that!

@wetzelj wetzelj mentioned this pull request Sep 30, 2020
Closed
@vsoch vsoch deleted the fixes-to-detect-clean branch September 10, 2022 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@vsoch @wetzelj