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

Update docstrings formatting refactor #255

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Update docstrings formatting refactor #255

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9050202
add params to docstrings for add_tag find_tag
bushjam1 Jun 14, 2023
912e3a8
refactor apply_filter() switch statement to dict map
bushjam1 Jun 14, 2023
45a55f2
refactor lines in excess of 100 chars
bushjam1 Jun 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 14 additions & 24 deletions deid/dicom/filter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,37 +26,27 @@ def apply_filter(dicom, field, filter_name, value):
dicom: the pydicom.dataset Dataset (pydicom.read_file)
field: the name of the field to apply the filter to,
or the tag number as a string '0xGGGGEEEE'
filer_name: the name of the filter to apply (e.g., contains)
filter_name: the name of the filter to apply (e.g., contains)
value: the value to set, if filter_name is valid

"""
if "0x" in field:
field = int(field, 0) # 0=decode hex with 0x prefix
filter_name = filter_name.lower().strip()
filter_name_map = {
Copy link
Member

Choose a reason for hiding this comment

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

I see what you are trying to do, but this design isn't an improvement because in creating the dictionary you are executing each function. We definitely don't want to do that.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for the feedback.

Copy link
Member

Choose a reason for hiding this comment

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

If you’d like to revert this change, the docstring fixes are great and would be much appreciated!

"contains": dicom.contains(field, value),
"notcontains": dicom.notContains(field, value),
"equals": dicom.equals(field, value),
"missing": notdicom.missing(field),
"present": not dicom.missing(field),
"empty": dicom.empty(field),
"notequals": dicom.notEquals(field, value)
}
if filter_name not in filter_name_map.keys():
bot.warning("%s is not a valid filter name, returning False" % filter_name)
return False

if filter_name == "contains":
return dicom.contains(field, value)

if filter_name == "notcontains":
return dicom.notContains(field, value)

elif filter_name == "equals":
return dicom.equals(field, value)

elif filter_name == "missing":
return dicom.missing(field)

elif filter_name == "present":
return not dicom.missing(field)

elif filter_name == "empty":
return dicom.empty(field)

elif filter_name == "notequals":
return dicom.notEquals(field, value)

bot.warning("%s is not a valid filter name, returning False" % filter_name)
return False
return filter_name_map[filter_name]


################################################################################
Expand Down
17 changes: 10 additions & 7 deletions deid/dicom/parser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,9 +295,10 @@ def keep(self):
@property
def excluded_from_deletion(self):
"""
Return once-evaluated list of fields that are not removed by REMOVE ALL or REMOVE SomeField,
as they later have to be changed by REPLACE / JITTER
That allows whitelisting fields from REMOVE ALL/SomeField to change them if needed (i.e. obfuscation)
Return once-evaluated list of fields that are not removed by
REMOVE ALL or REMOVE SomeField, as they later have to be
changed by REPLACE / JITTER. That allows whitelisting fields
from REMOVE ALL/SomeField to change them if needed (i.e. obfuscation)
"""
if self._excluded_fields is None:
self._excluded_fields = []
Expand Down Expand Up @@ -397,10 +398,12 @@ def perform_action(self, field, value, action, filemeta=False):
fields = self.find_by_values(values=values)

# A fields list is used verbatim
# In expand_field_expression below, the stripped_tag is being passed in to field. At this point,
# expanders for %fields lists have already been processed and each of the contenders is an
# identified, unique field. It is important to use stripped_tag at this point instead of
# element.keyword as private tags will not have a keyword and can only be identified by tag number.
# In expand_field_expression below, the stripped_tag is being passed
# in to field. At this point, expanders for %fields lists have already
# been processed and each of the contenders is an identified, unique
# field. It is important to use stripped_tag at this point instead of
# element.keyword as private tags will not have a keyword and can
# only be identified by tag number.
elif re.search("^fields", field):
listing = {}
for uid, contender in self.lookup.get(
Expand Down
21 changes: 18 additions & 3 deletions deid/dicom/tags.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,16 @@


def add_tag(identifier, VR="ST", VM=None, name=None, keyword=None):
"""Add tag will take a string for a tag (e.g., ) and define a new tag for it.
"""add_tag will take a string for a tag (e.g., ) and define a new tag for it.
By default, we give the type "Short Text."

Parameters
==========
identifier: string attribute identifier - 0001 in (0009, 0001)
VR: value representation of data element (default None)
VM value multiplicity of data element (default None)
name: data element name (default None )
keyword: data element keyword (default None)
"""
tag = Tag("0x" + identifier)
manifest = {
Expand Down Expand Up @@ -64,6 +72,13 @@ def get_tag(field):
def find_tag(term, VR=None, VM=None, retired=False):
"""find_tag will search over tags in the DicomDictionary and return the tags found
to match some term.

Parameters
==========
term: string supplied for search
VR: value representation of data element (default None)
VM: value multiplicity of data element (default None)
retired: searched data element is retired (default False)
"""
searchin = DicomDictionary
if retired:
Expand All @@ -84,7 +99,7 @@ def find_tag(term, VR=None, VM=None, retired=False):


def _filter_tags(tags, idx, fields=None):
"""filter tags is a helper function to take some list of tags in the format
"""filter_tags is a helper function to take some list of tags in the format
[ (VR, VM, longname, retired, keyword).. ]
where each of the items above has some index, idx, and filter that index
down to what is provided in fields.
Expand Down Expand Up @@ -114,7 +129,7 @@ def remove_sequences(dicom):


def update_tag(dicom, field, value):
"""update tag will update a value in the header, if it exists
"""update_tag will update a value in the header, if it exists
if not, nothing is added. This check is the only difference
between this function and change_tag.
If the user wants to add a value (that might not exist)
Expand Down