Skip to content
This repository has been archived by the owner on Feb 22, 2020. It is now read-only.

Improve various code styles #309

Merged
merged 23 commits into from
Mar 1, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
58b9a2c
No need for lists; prefer tuples as they imply we can't/won't change …
lamby Feb 25, 2018
ece38c0
Tidy """-style docstrings, moving everything to a vaguely-consistent …
lamby Feb 25, 2018
499a14c
Tidy code style around imports.
lamby Feb 25, 2018
fe5acc8
Reflow indentation to prevent unnecessary VCS diff noise on future ch…
lamby Feb 25, 2018
5302cfe
Import os.path.{join,dirname,abspath} directly instead of importing `…
lamby Feb 25, 2018
bfbd05a
Import "batteries-included" modules as top-level, ie. without using `…
lamby Feb 25, 2018
e0e9fe4
Drop unnecessary parenthesis.
lamby Feb 25, 2018
c9a1f2e
Use [] and {} to generate empty lists and dictionaries over list() an…
lamby Feb 25, 2018
833c810
Use `os.path.join` over constructing paths ourselves.
lamby Feb 25, 2018
b799d19
Drop print() statement.
lamby Feb 25, 2018
f27e617
Avoid aliasing __builtins__.
lamby Feb 25, 2018
e297f78
Define `config` dicts in one go, instead of poking things into them a…
lamby Feb 25, 2018
ceb9269
Make it clearer that `sizelim` is a float; "6." is not very obvious!
lamby Feb 25, 2018
5b8f54b
Prefer single quotes over double quotes; the former expresses better …
lamby Feb 25, 2018
972d25f
Drop unnecessary parenthesis.
lamby Feb 25, 2018
8fa8a63
Use the `exist_ok` keyword argument when calling `os.makedirs` instea…
lamby Feb 26, 2018
119a6b6
Use more Pythonic `num_X` variable names over `num_of_X` or `X_count`.
lamby Feb 26, 2018
a8bf16f
Improve code spacing variously for readability.
lamby Feb 25, 2018
ed2c8a7
Use str.format over interpolation with '%'.
lamby Feb 26, 2018
de85306
Drop weird/misleading/commented-out code.
lamby Feb 26, 2018
2558c7c
Tidy "inline" comments.
lamby Feb 25, 2018
4b05193
Cleanup/remove some unnecessary control-flow statement around uncondi…
lamby Feb 25, 2018
0477a6a
Prevent a long line warning.
lamby Feb 27, 2018
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
9 changes: 7 additions & 2 deletions docs/conf.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import os
import sys
import django

from recommonmark.transform import AutoStructify

DOCS_DIR = os.getcwd()
Expand Down Expand Up @@ -73,8 +74,12 @@
# directories to ignore when looking for source files.
# This patterns also effect to html_static_path and html_extra_path
exclude_patterns = [
'_build', 'Thumbs.db', '.DS_Store', 'apidoc_interface/modules.rst',
'apidoc_prediction/modules.rst']
'_build',
'Thumbs.db',
'.DS_Store',
'apidoc_interface/modules.rst',
'apidoc_prediction/modules.rst',
]

# The name of the Pygments (syntax highlighting) style to use.
pygments_style = 'sphinx'
Expand Down
25 changes: 15 additions & 10 deletions interface/backend/api/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,13 @@


class ImageFileSerializer(serializers.ModelSerializer):
""" Serializes an ImageFile model object.
"""
Serializes an ImageFile model object.
In addition to the properties on the object, it returns a `preview_url` which
will return the dicom image data encoded in base64.
In addition to the properties on the object, it returns a `preview_url`
which will return the dicom image data encoded in base64.
"""

class Meta:
model = ImageFile
fields = '__all__'
Expand All @@ -31,11 +33,13 @@ def get_preview_url(self, instance):


class ImageSeriesSerializer(serializers.HyperlinkedModelSerializer):
""" Serializes an ImageSeries model object.
"""
Serializes an ImageSeries model object.
Will also contain an `images` property with all of the serialized
image files in this series.
Will also contain an `images` property with all of the serialized image
files in this series.
"""

class Meta:
model = ImageSeries
fields = '__all__'
Expand Down Expand Up @@ -97,12 +101,13 @@ def create(self, validated_data):


class CaseSerializer(serializers.HyperlinkedModelSerializer):
""" Serializes a Case model object
"""
Serializes a Case model object
Case is the top level of the object hierarchy, and it contains
the image series, candidates, and nodules for the currently active
case.
Case is the top level of the object hierarchy, and it contains the
image series, candidates, and nodules for the currently active case.
"""

class Meta:
model = Case
fields = '__all__'
Expand Down
6 changes: 4 additions & 2 deletions interface/backend/api/tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -115,10 +115,12 @@ def test_candidates_dismiss(self):


class SerializerTest(TestCase):
""" Tests the serializers that perform any custom logic.
"""
Tests the serializers that perform any custom logic.
In general, create logic is tested by posting in the ViewTest test cases.
In general, create logic is tested by posting in the ViewTest test cases.
"""

def test_image_file_serializer(self):
image_file = ImageFile(path='/my_path')
serialized = ImageFileSerializer(image_file)
Expand Down
8 changes: 5 additions & 3 deletions interface/backend/api/urls.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from collections import OrderedDict
import collections
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you prefer to import the whole module here while you changed in 5302cfe to directly import methods from a module?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So in general I prefer to import the modules that Python ships with ("batteries included") as top-level modules. However, I am not 100% dogmatic on this (!) and make some exceptions if that's going to make the code unspeakably ugly. For example,, imagine trying to parse:

os.path.join(os.path.join(os.path.join(os.path.ditpath(os.path.join(os.path.join(os.path.join(....)))

Make sense? :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, thank you :)


from backend.api.views import (
CaseViewSet,
Expand All @@ -21,11 +21,13 @@


class RelativeUrlRootView(routers.APIRootView):
""" Provides relative URLs for the available endpoints.
"""
Provides relative URLs for the available endpoints.
"""

def get(self, request, *args, **kwargs):
# Return a plain {"name": "hyperlink"} response.
ret = OrderedDict()
ret = collections.OrderedDict()
namespace = request.resolver_match.namespace
for key, url_name in self.api_root_dict.items():
if namespace:
Expand Down
6 changes: 3 additions & 3 deletions interface/backend/images/factories.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,11 @@ class ImageSeriesFactory(factory.django.DjangoModelFactory):
class Meta:
model = models.ImageSeries

patient_id = factory.Sequence(lambda n: "TEST-SERIES-%04d" % n)
patient_id = factory.Sequence(lambda n: "TEST-SERIES-{:4d}".format(n))

series_instance_uid = factory.Sequence(lambda n: "1.3.6.1.4.1.14519.5.2.1.6279.6001.%030d" % n)
series_instance_uid = factory.Sequence(lambda n: "1.3.6.1.4.1.14519.5.2.1.6279.6001.{:30d}".format(n))

uri = factory.LazyAttribute(lambda f: 'file:///tmp/%s/' % f.series_instance_uid)
uri = factory.LazyAttribute(lambda f: 'file:///tmp/{}/'.format(f.series_instance_uid))


class ImageLocationFactory(factory.django.DjangoModelFactory):
Expand Down
102 changes: 62 additions & 40 deletions interface/backend/images/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,11 @@


class ImageFile(models.Model):
""" Model for an individual file for an image--that is, a single DICOM slice
as it is represented on disk.
"""
Model for an individual file for an image--that is, a single DICOM slice as
it is represented on disk.
"""

# Explicitly map all the dicom properties that we need to parse and their
# model equivalents; create a lambda that returns a dict for any type conversions
# or one-to-many relationships
Expand All @@ -35,42 +37,47 @@ class ImageFile(models.Model):
columns = models.IntegerField(null=True)
pixel_spacing_col = models.FloatField(null=True)
pixel_spacing_row = models.FloatField(null=True)
path = models.FilePathField(path=settings.DATASOURCE_DIR,
recursive=True,
allow_files=True,
allow_folders=False,
max_length=512)
path = models.FilePathField(
path=settings.DATASOURCE_DIR,
recursive=True,
allow_files=True,
allow_folders=False,
max_length=512,
)

class Meta:
ordering = ['slice_location']
ordering = ('slice_location',)

def save(self, *args, **kwargs):
self._populate_dicom_properties()

super().save(*args, **kwargs)

def _populate_dicom_properties(self):
""" Parse DICOM properties from the file and store the relevant ones
on the object.
"""
Parse DICOM properties from the file and store the relevant ones on the
object.
"""

logger = logging.getLogger(__name__)
for k, v in self.load_dicom_data_from_disk(self.path)['metadata'].items():
# check that the field is a property on the object
if not hasattr(self.__class__, k):
logger.warning(f"Property '{k}' not a field on ImageFile, discarding value '{v}'.")

else:
if hasattr(self.__class__, k):
self.__setattr__(k, v)
else:
logger.warning(f"Property '{k}' not a field on ImageFile, discarding value '{v}'.")

@classmethod
def load_dicom_data_from_disk(cls, filepath, parse_metadata=True, encode_image_data=False):
""" Loads the dicom properties that we care about from disk.
"""
Loads the dicom properties that we care about from disk.
For the values we load, see ImageFile.DICOM_PROPERTIES
For the values we load, see ImageFile.DICOM_PROPERTIES
Args:
filepath (str): the path to the file (so this can be used if ImageFile is not in database yet)
parse_metadata (bool, optional): weather or not to parse any metadata from the file (skip for just img)
encode_image_data (bool, optional): Base64 encode image data and return it.
Args:
filepath (str): the path to the file (so this can be used if ImageFile is not in database yet)
parse_metadata (bool, optional): weather or not to parse any metadata from the file (skip for just img)
encode_image_data (bool, optional): Base64 encode image data and return it.
"""
if not os.path.abspath(filepath).startswith(settings.DATASOURCE_DIR):
raise PermissionDenied
Expand All @@ -87,8 +94,9 @@ def load_dicom_data_from_disk(cls, filepath, parse_metadata=True, encode_image_d

@classmethod
def _parse_metadata(cls, dcm_data, filepath):
""" Parses metadata from dicom file and creates a dictionary
which we can use to populate the ImageFile object
"""
Parses metadata from dicom file and creates a dictionary which we can
use to populate the ImageFile object
"""
logger = logging.getLogger(__name__)

Expand All @@ -101,7 +109,7 @@ def _update_once(d1, d2):
else:
d1[k] = v

metadata = dict()
metadata = {}
for dicom_prop, model_attribute in cls.DICOM_PROPERTIES.items():
if callable(model_attribute):
try:
Expand All @@ -117,7 +125,9 @@ def _update_once(d1, d2):
return metadata

def get_image_data(self):
""" Shortcut for just the encoded image from `load_dicom_properties_from_disk`.
"""
Shortcut for just the encoded image from
`load_dicom_properties_from_disk`.
"""
return self.load_dicom_data_from_disk(self.path, parse_metadata=False, encode_image_data=True)['image']

Expand All @@ -133,6 +143,7 @@ def _dicom_to_base64(cls, ds):
Returning base64 encoded string for a dicom image
"""
rescaled = cls._pixel_data2str(ds.pixel_array)

return base64.b64encode(rescaled.tobytes())


Expand All @@ -147,33 +158,43 @@ class ImageSeries(models.Model):
@classmethod
def get_or_create(cls, uri):
"""
Return the ImageSeries instance with the same PatientID and SeriesInstanceUID as the DICOM images in the
given directory. If none exists so far, create one.
Return a tuple of (ImageSeries, created), where created is a boolean specifying whether the object was created.
Return the ImageSeries instance with the same PatientID and
SeriesInstanceUID as the DICOM images in the given directory. If none
exists so far, create one.
Returns a tuple of (ImageSeries, created), where created is a boolean
specifying whether the object was created.
Args:
uri (str): absolute URI to a directory with DICOM images of a patient
uri (str): absolute URI to a directory with DICOM images of a
patient
Returns:
(ImageSeries, bool): the looked up ImageSeries instance and whether it had to be created
(ImageSeries, bool): the looked up ImageSeries instance and whether
it had to be created
"""
# get all the images in the folder that are valid dicom extensions
files = [f for f in os.listdir(uri)
if os.path.splitext(f)[-1] in settings.IMAGE_EXTENSIONS]

# load series-level metadata from the first dicom file
# Get all the images in the folder that are valid dicom extensions
files = [
f for f in os.listdir(uri)
if os.path.splitext(f)[-1] in settings.IMAGE_EXTENSIONS
]

# Load series-level metadata from the first dicom file
plan = dicom.read_file(safe_join(uri, files[0]))

patient_id = plan.PatientID
series_instance_uid = plan.SeriesInstanceUID

series, created = ImageSeries.objects.get_or_create(patient_id=patient_id,
series_instance_uid=series_instance_uid,
uri=uri)
series, created = ImageSeries.objects.get_or_create(
patient_id=patient_id,
series_instance_uid=series_instance_uid,
uri=uri,
)

# create models that point to each of the individual image files
for f in files:
image, _ = ImageFile.objects.get_or_create(path=safe_join(uri, f), series=series)
# Create models that point to each of the individual image files
for x in files:
image, _ = ImageFile.objects.get_or_create(path=safe_join(uri, x), series=series)

return series, created

Expand All @@ -183,8 +204,9 @@ def __str__(self):

class ImageLocation(models.Model):
"""
Model representing a certain voxel location on certain image
Model representing a certain voxel location on certain image.
"""

x = models.PositiveSmallIntegerField(help_text='Voxel index for X axis, zero-index, from top left')
y = models.PositiveSmallIntegerField(help_text='Voxel index for Y axis, zero-index, from top left')
z = models.PositiveSmallIntegerField(help_text='Slice index for Z axis, zero-index')
1 change: 1 addition & 0 deletions interface/config/settings/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
For the full list of settings and their values, see
https://docs.djangoproject.com/en/1.11/ref/settings/
"""

import environ

BASE_DIR = environ.Path(__file__) - 3
Expand Down
1 change: 1 addition & 0 deletions interface/config/settings/test.py
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@
# ------------------------------------------------------------------------------
# Keep templates in memory so tests run faster
TEMPLATES[0].pop('APP_DIRS', None)

TEMPLATES[0]['OPTIONS']['loaders'] = [
['django.template.loaders.cached.Loader', [
'django.template.loaders.filesystem.Loader',
Expand Down
34 changes: 16 additions & 18 deletions prediction/config.py
Original file line number Diff line number Diff line change
@@ -1,31 +1,29 @@
"""
prediction.config
~~~~~~~~~~~~~~~~~
Provides the flask config options
Provides the flask config options
"""

import os

from os import path
from os.path import abspath, dirname, join, realpath

LIDC_WILDCARD = ['LIDC-IDRI-*', '**', '**']
LIDC_WILDCARD = ('LIDC-IDRI-*', '**', '**')


class Config(object):
PROD_SERVER = os.getenv('PRODUCTION', False)
DEBUG = False
CURRENT_DIR = path.dirname(path.realpath(__file__))
PARENT_DIR = path.dirname(CURRENT_DIR)
ALGOS_DIR = path.abspath(path.join(CURRENT_DIR, 'src', 'algorithms'))
ALGO_CLASSIFY_GTR123_PATH = os.path.join(ALGOS_DIR, 'classify', 'assets', 'gtr123_model.ckpt')
SEGMENT_ASSETS_DIR = path.abspath(path.join(ALGOS_DIR, 'segment', 'assets'))
ALGO_SEGMENT_PATH = os.path.join(SEGMENT_ASSETS_DIR, 'best_model.hdf5')
FULL_DICOM_PATHS = path.join(PARENT_DIR, 'images_full')
SMALL_DICOM_PATHS = path.join(PARENT_DIR, 'images')
FULL_DICOM_PATHS_WILDCARD = path.join(FULL_DICOM_PATHS, *LIDC_WILDCARD)
SMALL_DICOM_PATHS_WILDCARD = path.join(FULL_DICOM_PATHS, *LIDC_WILDCARD)
DATA_DIR = path.abspath(path.join(CURRENT_DIR, 'data'))
EXTRACTED_IMAGE_DIR = path.abspath(path.join(CURRENT_DIR, 'extracted'))
CURRENT_DIR = dirname(realpath(__file__))
PARENT_DIR = dirname(CURRENT_DIR)
ALGOS_DIR = abspath(join(CURRENT_DIR, 'src', 'algorithms'))
ALGO_CLASSIFY_GTR123_PATH = join(ALGOS_DIR, 'classify', 'assets', 'gtr123_model.ckpt')
SEGMENT_ASSETS_DIR = abspath(join(ALGOS_DIR, 'segment', 'assets'))
ALGO_SEGMENT_PATH = join(SEGMENT_ASSETS_DIR, 'best_model.hdf5')
FULL_DICOM_PATHS = join(PARENT_DIR, 'images_full')
SMALL_DICOM_PATHS = join(PARENT_DIR, 'images')
FULL_DICOM_PATHS_WILDCARD = join(FULL_DICOM_PATHS, *LIDC_WILDCARD)
SMALL_DICOM_PATHS_WILDCARD = join(FULL_DICOM_PATHS, *LIDC_WILDCARD)
DATA_DIR = abspath(join(CURRENT_DIR, 'data'))
EXTRACTED_IMAGE_DIR = abspath(join(CURRENT_DIR, 'extracted'))


class Production(Config):
Expand Down
Loading