CLI 'pixel' command #285
Open
CLI 'pixel' command #285
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WillForan
force-pushed
the
cli-pixelclean
branch
2 times, most recently
from
ff05ed4 to
981b1de
Compare
has_burn_in is never used, so dont save it 'import os' unnecessary
WillForan
force-pushed
the
cli-pixelclean
branch
from
981b1de to
2e36cf3
Compare
vsoch
reviewed
Sep 4, 2025
WillForan
force-pushed
the
cli-pixelclean
branch
from
72fa123 to
89bf7cb
Compare
This could be a breaking change if it `image_type='original'` was ever valid. I don't think it is (more below). `image_type` can/should likely be removed from all other save_* functions too. Ostensibly, the option existed to look into the original pixel_array. But `clean_pixel_data` is the only place I see with that variable and it is run as an isolated function w/ no access to the DicomCleaner object. DicomCleaner has no `original` attribute. ``` (Pdb) self.__dir__() ['font', 'cmap', 'output_folder', 'recipe', 'results', 'force', 'dicom_file', 'cleaned', '__module__', '__doc__', '__init__', 'default_font', 'detect', 'clean', 'get_figure', '_get_clean_name', 'save_png', 'save_animation', 'save_dicom', '__dict__', '__weakref__', '__new__', '__repr__', '__hash__', '__str__', '__getattribute__', '__setattr__', '__delattr__', '__lt__', '__le__', '__eq__', '__ne__', '__gt__', '__ge__', '__reduce_ex__', '__reduce__', '__getstate__', '__subclasshook__', '__init_subclass__', '__format__', '__sizeof__', '__dir__', '__class__'] (Pdb) self.original *** AttributeError: 'DicomCleaner' object has no attribute 'original' ````
WillForan
force-pushed
the
cli-pixelclean
branch
from
a9aa79d to
910c5d8
Compare
WillForan
force-pushed
the
cli-pixelclean
branch
from
cb32ca2 to
fe724de
Compare
|
I think I'm all done with the random force pushes (sorry!). Anything still outstanding? |
|
This is looking good! Some final points I think:
# create command groups up here...
for command in [inspect, clean_pixels, etc]:
# add the same argument here for deid, ids, etc.Finally, we should have one more set of eyes, although the PR is fairly straight forward. Pinging @wetzelj if you are still around, but any contributor will do! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Minimal implementation of a
clean-pixelscommand option for the CLI (#284).clean-prefix and no extension change for predicable dicom_save (new directory only, not new names). Not sure if that's wise -- just how I'd originally written the test and another user might expect output data.DicomCleanerdetect, clean, save_dicom on input directoryI think it'd be useful to do both header and pixel scrubbing in one interface so everything described in a deid profile/config file can be applied at once. I'm not sure yet what the best way to go about that is (run identifiers into a new temp dir, rerun clean on that and save output to final location?)