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

Fix LGTM alerts #186

Merged
merged 3 commits into from
Oct 7, 2021
Merged

Fix LGTM alerts #186

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
45560a4
LGTM warning: Unguarded next in generator
DimitriPapadopoulos Sep 10, 2021
e3b6203
LGTM recommendation: Unused import
DimitriPapadopoulos Sep 10, 2021
e48adbc
LGTM recommendation: Except block handles 'BaseException'
DimitriPapadopoulos Sep 10, 2021
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
1 change: 0 additions & 1 deletion deid/dicom/filter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
"""

from pydicom.dataset import Dataset
from pydicom.sequence import Sequence
from deid.logger import bot
import re

Expand Down
3 changes: 0 additions & 3 deletions deid/dicom/groups.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,9 @@


from deid.logger import bot
from .tags import remove_sequences
from .fields import get_fields, expand_field_expression
from pydicom.multival import MultiValue

import os


def extract_values_list(dicom, actions, fields=None):
"""Given a list of actions for a named group (a list) extract values from
Expand Down
7 changes: 1 addition & 6 deletions deid/dicom/header.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,16 +26,11 @@


from deid.logger import bot
from deid.utils import read_json

from deid.config import DeidRecipe

from pydicom import read_file

from deid.dicom.utils import save_dicom
from deid.dicom.tags import remove_sequences, get_private
from deid.dicom.groups import extract_values_list, extract_fields_list
from deid.dicom.fields import get_fields
from deid.dicom.tags import remove_sequences
from deid.dicom.parser import DicomParser

import os
Expand Down
1 change: 0 additions & 1 deletion deid/dicom/tags.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
from deid.logger import bot
from pydicom.tag import tag_in_exception
from pydicom.sequence import Sequence
from pydicom.dataelem import DataElement
from pydicom._dicom_dict import DicomDictionary, RepeatersDictionary
from pydicom.tag import Tag
import re
Expand Down
32 changes: 17 additions & 15 deletions deid/dicom/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,21 +64,23 @@ def get_files(contenders, check=True, pattern=None, force=False, tempdir=None):
if dextension == ".zip":
with zipfile.ZipFile(dicom_file, "r") as compressedFile:
compressedFile.extractall(tempdir)
dicom_file = next(
os.path.join(tempdir, f)
for f in os.listdir(tempdir)
if os.path.isfile(os.path.join(tempdir, f))
)

if dicom_file is not None:
Copy link
Member

Choose a reason for hiding this comment

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

Looks like you forgot to add back the check here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not at all. It's useless. See commments in commit.

Copy link
Member

Choose a reason for hiding this comment

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

@BrianKolowitz it looks like you added this check - can you confirm the change looks okay to you?

Copy link
Contributor Author

@DimitriPapadopoulos DimitriPapadopoulos Sep 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe it wasn't useless before, now it is useless because of the additional try/except.

Copy link
Contributor Author

@DimitriPapadopoulos DimitriPapadopoulos Sep 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

dicom_file cannot be empty because we continue in case of StopIteration - or an exception is raised in case of other erros.

Copy link
Member

Choose a reason for hiding this comment

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

Understood, and it LGTM - I'm just hoping to get the blessing of the original contributor before changing the code he wrote.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I'd like to have it too.

Copy link
Member

Choose a reason for hiding this comment

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

@wetzelj it looks like @BrianKolowitz is not able to respond (I hope he's okay!) and I'd like to get one more review on this before merge. It's fairly small changes - if you have time in the next few weeks could you take a look?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for my delayed response... I was out of the office last week and have been playing catch up.

Looking at this change, it looks like it should be okay. Since the next() will raise StopIteration when the iterator is exhausted (i.e. there are no items left in the zipfile), the continue statement that you've put in on StopIteration will continue to the next item up for processing in the list loop on line 60 (iterating through multiple files/directories).

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @wetzelj ! That is my sentiment as well.

if check:
validated_files = validate_dicoms(dicom_file, force=force)
else:
validated_files = [dicom_file]

for validated_file in validated_files:
bot.debug("Found contender file %s" % (validated_file))
yield validated_file
try:
dicom_file = next(
os.path.join(tempdir, f)
for f in os.listdir(tempdir)
if os.path.isfile(os.path.join(tempdir, f))
)
except StopIteration:
continue # ZIP file does not contain any file

if check:
validated_files = validate_dicoms(dicom_file, force=force)
else:
validated_files = [dicom_file]

for validated_file in validated_files:
bot.debug("Found contender file %s" % (validated_file))
yield validated_file


def save_dicom(dicom, dicom_file, output_folder=None, overwrite=False):
Expand Down
2 changes: 1 addition & 1 deletion deid/utils/actions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ def get_timestamp(item_date, item_time=None, jitter_days=None, format=None):

try:
timestamp = dateutil.parser.parse("%s%s" % (item_date, item_time))
except:
except (dateutil.parser.ParserError, OverflowError):
timestamp = datetime.strptime("%s%s" % (item_date, item_time), format)

if jitter_days is not None:
Expand Down
1 change: 0 additions & 1 deletion examples/dicom/dicom-extract/create-dicom-csv.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
import os
import platform
import csv
import operator
from collections.abc import Sequence
from collections import OrderedDict

Expand Down
2 changes: 0 additions & 2 deletions examples/dicom/header-manipulation/func-replacement.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
#!/usr/bin/env python3

from deid.dicom import get_files, replace_identifiers
from deid.utils import get_installdir
from deid.data import get_dataset
import os

# This is an example of replacing fields in dicom headers,
# but via a function instead of a preset identifier.
Expand Down
2 changes: 0 additions & 2 deletions examples/dicom/pixels/run-inspect-pixels.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,8 @@

# This will get a set of example cookie dicoms
from deid.dicom import get_files, has_burned_pixels
from pydicom import read_file
from deid.data import get_dataset
from deid.logger import bot
import os

bot.level = 3

Expand Down