diff --git a/.github/workflows/pythonpackage.yml b/.github/workflows/pythonpackage.yml index 71cdd26..8a09dc6 100644 --- a/.github/workflows/pythonpackage.yml +++ b/.github/workflows/pythonpackage.yml @@ -3,12 +3,16 @@ name: Python tests on: [push, pull_request] jobs: - build: + test: runs-on: ${{ matrix.os }} strategy: + fail-fast: false matrix: - os: [ubuntu-latest, macOS-latest, windows-2016] - python-version: [3.6, 3.7, 3.8, 3.9] + os: [ubuntu-latest, macOS-latest, windows-latest] + python-version: [3.6, 3.7, 3.8, 3.9, "3.10"] + exclude: + - os: macOS-latest + python-version: 3.6 steps: - uses: actions/checkout@v1 - name: Set up Python ${{ matrix.python-version }} diff --git a/.landscape.yml b/.landscape.yml index ceb4e64..ebf0424 100644 --- a/.landscape.yml +++ b/.landscape.yml @@ -1,4 +1,3 @@ doc-warnings: false test-warnings: true -strictness: high -max-line-length: 120 \ No newline at end of file +strictness: high \ No newline at end of file diff --git a/.pylintrc b/.pylintrc index 55cdfc3..4d44281 100644 --- a/.pylintrc +++ b/.pylintrc @@ -2,9 +2,6 @@ # disable check C0111 for doc strings disable=C0111 -[FORMAT] -max-line-length=120 - [BASIC] good-names=i,j,k,ex,_,setUp,setUpClass diff --git a/README.md b/README.md index 51e4f72..0b220bf 100644 --- a/README.md +++ b/README.md @@ -2,12 +2,14 @@ [![PyPI version](https://badge.fury.io/py/dicom-validator.svg)](https://pypi.org/project/dicom-validator) ![Python package](https://github.com/pydicom/dicom-validator/workflows/Python%20package/badge.svg) [![Python version](https://img.shields.io/pypi/pyversions/dicom-validator.svg)](https://pypi.org/project/dicom-validator) -*dicom-validator* was planned to be a collection of simple pure python command line tools which get the input from -the DICOM standard in docbook format as provided by [ACR NEMA](http://medical.nema.org/). -As the only relevant tool is the validator tool, and no further tools are planned, the package has now been renamed -from the original name *dcm-spec-tools*, and moved to the pydicom organization. - -Currently available tools: +*dicom-validator* was originally planned to be a collection of simple pure +python command line tools which get the input from the DICOM standard in +docbook format as provided by [ACR NEMA](http://medical.nema.org/). +As the only relevant tool is the validator tool, and no further tools are +planned, the package has now been renamed from the original name +*dcm-spec-tools*, and moved to the pydicom organization. + +Available tools: * `validate_iods` checks DICOM files for correct attributes for the given SOP class. * `dump_dcm_info` outputs the DICOM tag IDs and values of a given DICOM file (used as a simple demonstration for access to the DICOM standard). @@ -36,10 +38,10 @@ Access to DICOM standard ------------------------ Upon first start of a tool, part of the latest version of the DICOM standard -in docbook format is downloaded, parsed, and the needed information saved in -json files. These files are then used by the tools. Periodically (once a -month), the tools check for a newer version of the DICOM standard and download -it if found. +in docbook format (specifically, parts 3.3, 3.4 and 3.6) is downloaded, +parsed, and the needed information saved in json files. +These files are then used by the tools. Periodically (once a month), the tools +check for a newer version of the DICOM standard and download it if found. It is also possible to use older versions of the standard via a command line option, provided they are still available for download (at the time of @@ -134,4 +136,4 @@ Tag (0040,A13A) (Referenced DateTime) is missing due to condition: ``` *Note:* No guarantees are given for the correctness of the results. This is -pre-alpha software and mostly thought as a proof of concept. \ No newline at end of file +alpha-stage software and mostly thought as a proof of concept. \ No newline at end of file diff --git a/dcm_spec_tools/dump_dcm_info.py b/dcm_spec_tools/dump_dcm_info.py index e422d0e..1befe57 100644 --- a/dcm_spec_tools/dump_dcm_info.py +++ b/dcm_spec_tools/dump_dcm_info.py @@ -3,11 +3,10 @@ """ import argparse -import json import os import re -from pydicom import filereader +from pydicom import dcmread from pydicom.errors import InvalidDicomError from dcm_spec_tools.spec_reader.edition_reader import EditionReader @@ -116,7 +115,7 @@ def print_sequence(self, sequence): def dump_file(self, file_path): try: print('\n' + file_path) - dataset = filereader.read_file( + dataset = dcmread( file_path, stop_before_pixels=self.show_image_data, force=True) self.print_dataset(dataset) except (InvalidDicomError, KeyError): @@ -167,13 +166,8 @@ def main(): return 1 json_path = os.path.join(destination, 'json') - with open(os.path.join(json_path, - edition_reader.dict_info_json)) as info_file: - dict_info = json.load(info_file) - with open(os.path.join(json_path, - edition_reader.uid_info_json)) as info_file: - uid_info = json.load(info_file) - + dict_info = EditionReader.load_dict_info(json_path) + uid_info = EditionReader.load_uid_info(json_path) dumper = DataElementDumper(dict_info, uid_info, args.max_value_len, args.show_image_data, args.show_tags) diff --git a/dcm_spec_tools/spec_reader/edition_reader.py b/dcm_spec_tools/spec_reader/edition_reader.py index 935f71d..c077928 100644 --- a/dcm_spec_tools/spec_reader/edition_reader.py +++ b/dcm_spec_tools/spec_reader/edition_reader.py @@ -168,6 +168,24 @@ def get_chapter(self, revision, chapter, destination, is_current): file_path)) return False + @staticmethod + def load_info(json_path, info_json): + with open(os.path.join(json_path, + info_json)) as info_file: + return json.load(info_file) + + @classmethod + def load_dict_info(cls, json_path): + return cls.load_info(json_path, cls.dict_info_json) + + @classmethod + def load_uid_info(cls, json_path): + return cls.load_info(json_path, cls.uid_info_json) + + @classmethod + def load_module_info(cls, json_path): + return cls.load_info(json_path, cls.module_info_json) + @classmethod def json_files_exist(cls, json_path): for filename in (cls.dict_info_json, cls.module_info_json, diff --git a/dcm_spec_tools/tag_tools.py b/dcm_spec_tools/tag_tools.py index ecc9ea1..4bd9eb8 100644 --- a/dcm_spec_tools/tag_tools.py +++ b/dcm_spec_tools/tag_tools.py @@ -1,6 +1,7 @@ def tag_name_from_id_string(tag_id_string, dict_info): if dict_info and tag_id_string in dict_info: - return '{} ({})'.format(tag_id_string, dict_info[tag_id_string]['name']) + return '{} ({})'.format( + tag_id_string, dict_info[tag_id_string]['name']) return tag_id_string diff --git a/dcm_spec_tools/tests/fixtures/dicom/rtdose.dcm b/dcm_spec_tools/tests/fixtures/dicom/rtdose.dcm new file mode 100644 index 0000000..21d43ae Binary files /dev/null and b/dcm_spec_tools/tests/fixtures/dicom/rtdose.dcm differ diff --git a/dcm_spec_tools/tests/test_utils.py b/dcm_spec_tools/tests/test_utils.py index 77a4c8c..e02c692 100644 --- a/dcm_spec_tools/tests/test_utils.py +++ b/dcm_spec_tools/tests/test_utils.py @@ -11,3 +11,7 @@ def spec_fixture_path(): def json_fixture_path(): return os.path.join(fixture_path(), 'json') + + +def dicom_fixture_path(): + return os.path.join(fixture_path(), 'dicom') diff --git a/dcm_spec_tools/validate_iods.py b/dcm_spec_tools/validate_iods.py index 2369a79..79005f7 100644 --- a/dcm_spec_tools/validate_iods.py +++ b/dcm_spec_tools/validate_iods.py @@ -1,36 +1,37 @@ import argparse -import json import logging import os -from dcm_spec_tools.validator.dicom_file_validator import DicomFileValidator from dcm_spec_tools.spec_reader.edition_reader import EditionReader +from dcm_spec_tools.validator.dicom_file_validator import DicomFileValidator def validate(args, base_path): json_path = os.path.join(base_path, 'json') - with open(os.path.join(json_path, EditionReader.dict_info_json)) as info_file: - dict_info = json.load(info_file) - with open(os.path.join(json_path, EditionReader.iod_info_json)) as info_file: - iod_info = json.load(info_file) - with open(os.path.join(json_path, EditionReader.module_info_json)) as info_file: - module_info = json.load(info_file) + dict_info = EditionReader.load_dict_info(json_path) + iod_info = EditionReader.load_uid_info(json_path) + module_info = EditionReader.load_module_info(json_path) log_level = logging.DEBUG if args.verbose else logging.INFO validator = DicomFileValidator(iod_info, module_info, dict_info, log_level) error_nr = 0 for dicom_path in args.dicomfiles: - error_nr += sum(len(error) for error in list(validator.validate(dicom_path).values())) + error_nr += sum(len(error) for error in + list(validator.validate(dicom_path).values())) return error_nr def main(): parser = argparse.ArgumentParser( description='Validates DICOM file IODs') - parser.add_argument('dicomfiles', help='Path(s) of DICOM files or directories to validate', + parser.add_argument('dicomfiles', + help='Path(s) of DICOM files or directories ' + 'to validate', nargs='+') parser.add_argument('--standard-path', '-src', - help='Base path with the DICOM specs in docbook and json format', - default=os.path.join(os.path.expanduser("~"), 'dicom-validator')) + help='Base path with the DICOM specs in docbook ' + 'and json format', + default=os.path.join(os.path.expanduser("~"), + 'dicom-validator')) parser.add_argument('--revision', '-r', help='Standard revision (e.g. "2014c"), year of ' 'revision, "current" or "local" (latest ' @@ -43,7 +44,8 @@ def main(): edition_reader = EditionReader(args.standard_path) destination = edition_reader.get_revision(args.revision) if destination is None: - print('Failed to get DICOM edition {} - aborting'.format(args.revision)) + print( + 'Failed to get DICOM edition {} - aborting'.format(args.revision)) return 1 return validate(args, destination) diff --git a/dcm_spec_tools/validator/dicom_file_validator.py b/dcm_spec_tools/validator/dicom_file_validator.py index 26be196..7c8fb9b 100644 --- a/dcm_spec_tools/validator/dicom_file_validator.py +++ b/dcm_spec_tools/validator/dicom_file_validator.py @@ -2,14 +2,15 @@ import os import sys -from pydicom import filereader +from pydicom import dcmread from pydicom.errors import InvalidDicomError from dcm_spec_tools.validator.iod_validator import IODValidator class DicomFileValidator(object): - def __init__(self, iod_info, module_info, dict_info=None, log_level=logging.INFO): + def __init__(self, iod_info, module_info, dict_info=None, + log_level=logging.INFO): self._module_info = module_info self._iod_info = iod_info self._dict_info = dict_info @@ -40,10 +41,11 @@ def validate_dir(self, dir_path): def validate_file(self, file_path): self.logger.info('\nProcessing DICOM file "%s"', file_path) try: - data_set = filereader.read_file(file_path, stop_before_pixels=True) + data_set = dcmread(file_path, defer_size=1024) except InvalidDicomError: return {file_path: {'fatal': 'Invalid DICOM file'}} return { - file_path: IODValidator(data_set, self._iod_info, self._module_info, self._dict_info, + file_path: IODValidator(data_set, self._iod_info, + self._module_info, self._dict_info, self.logger.level).validate() } diff --git a/dcm_spec_tools/validator/iod_validator.py b/dcm_spec_tools/validator/iod_validator.py index 69ab7f9..d9b191e 100644 --- a/dcm_spec_tools/validator/iod_validator.py +++ b/dcm_spec_tools/validator/iod_validator.py @@ -3,7 +3,7 @@ import sys from dcm_spec_tools.spec_reader.condition import Condition -from dcm_spec_tools.tag_tools import tag_name_from_id_string, tag_name_from_id +from dcm_spec_tools.tag_tools import tag_name_from_id class InvalidParameterError(Exception): @@ -30,7 +30,8 @@ def validate(self): else: sop_class_uid = self._dataset.SOPClassUID if sop_class_uid not in self._iod_info: - self.errors['fatal'] = 'Unknown SOPClassUID (probably retired): ' + sop_class_uid + self.errors['fatal'] = (f'Unknown SOPClassUID ' + f'(probably retired): {sop_class_uid}') else: self._validate_sop_class(sop_class_uid) if 'fatal' in self.errors: @@ -82,11 +83,13 @@ def _validate_module(self, module, module_name): for tag_id_string, attribute in module_info.items(): tag_id = self._tag_id(tag_id_string) if tag_id in self._dataset: - message = self._incorrect_tag_message(tag_id, 'not allowed', None) + message = self._incorrect_tag_message(tag_id, + 'not allowed', None) errors.setdefault(message, []).append(tag_id_string) else: for tag_id_string, attribute in module_info.items(): - result = self._validate_attribute(self._tag_id(tag_id_string), attribute) + result = self._validate_attribute(self._tag_id(tag_id_string), + attribute) if result is not None: errors.setdefault(result, []).append(tag_id_string) return errors @@ -104,7 +107,8 @@ def _log_module_required(self, module_name, required, allowed, self.logger.debug(msg) def _incorrect_tag_message(self, tag_id, error_kind, condition_dict): - msg = 'Tag {} is {}'.format(tag_name_from_id(tag_id, self._dict_info), error_kind) + msg = 'Tag {} is {}'.format(tag_name_from_id(tag_id, self._dict_info), + error_kind) if condition_dict: condition = Condition.read_condition(condition_dict) if condition.type != 'U': @@ -125,7 +129,9 @@ def _validate_attribute(self, tag_id, attribute): elif attribute_type in ('1C', '2C'): if 'cond' in attribute: condition_dict = attribute['cond'] - tag_required, tag_allowed = self._object_is_required_or_allowed(condition_dict) + tag_required, tag_allowed = ( + self._object_is_required_or_allowed(condition_dict) + ) else: tag_required, tag_allowed = False, True else: @@ -133,7 +139,8 @@ def _validate_attribute(self, tag_id, attribute): error_kind = None if not has_tag and tag_required: error_kind = 'missing' - elif tag_required and value_required and self._dataset[tag_id].value is None: + elif (tag_required and value_required and + self._dataset[tag_id].value is None): error_kind = 'empty' elif has_tag and not tag_allowed: error_kind = 'not allowed' @@ -142,7 +149,6 @@ def _validate_attribute(self, tag_id, attribute): condition_dict) return msg - def _object_is_required_or_allowed(self, condition): if isinstance(condition, str): condition = json.loads(condition) @@ -157,9 +163,11 @@ def _object_is_required_or_allowed(self, condition): def _composite_object_is_required(self, condition): if 'and' in condition: - required = all(self._composite_object_is_required(cond) for cond in condition['and']) + required = all(self._composite_object_is_required(cond) + for cond in condition['and']) elif 'or' in condition: - required = any(self._composite_object_is_required(cond) for cond in condition['or']) + required = any(self._composite_object_is_required(cond) + for cond in condition['or']) else: required = self._object_is_required(condition) return required @@ -230,5 +238,6 @@ def _expanded_module_info(self, module_info): module_info.update(self._get_module_info(ref)) del module_info['include'] if 'items' in module_info: - module_info['items'] = self._expanded_module_info(module_info['items']) + module_info['items'] = self._expanded_module_info( + module_info['items']) return module_info diff --git a/dcm_spec_tools/validator/tests/test_dicom_file_validator.py b/dcm_spec_tools/validator/tests/test_dicom_file_validator.py index e3ab50c..36c1533 100644 --- a/dcm_spec_tools/validator/tests/test_dicom_file_validator.py +++ b/dcm_spec_tools/validator/tests/test_dicom_file_validator.py @@ -1,17 +1,19 @@ import json import logging import os +import unittest import pyfakefs.fake_filesystem_unittest from pydicom import write_file from pydicom.dataset import Dataset, FileDataset from dcm_spec_tools.spec_reader.edition_reader import EditionReader -from dcm_spec_tools.tests.test_utils import json_fixture_path +from dcm_spec_tools.tests.test_utils import json_fixture_path, \ + dicom_fixture_path from dcm_spec_tools.validator.dicom_file_validator import DicomFileValidator -class DicomFileValidatorTest(pyfakefs.fake_filesystem_unittest.TestCase): +class DicomFileValidatorTestBase(unittest.TestCase): iod_info = None module_info = None @@ -25,11 +27,19 @@ def setUpClass(cls): cls.module_info = json.load(info_file) def setUp(self): - super(DicomFileValidatorTest, self).setUp() - self.setUpPyfakefs() logging.disable(logging.CRITICAL) self.validator = DicomFileValidator(self.iod_info, self.module_info) + +class FakeDicomFileValidatorTest(DicomFileValidatorTestBase, + pyfakefs.fake_filesystem_unittest.TestCase): + iod_info = None + module_info = None + + def setUp(self): + super(FakeDicomFileValidatorTest, self).setUp() + self.setUpPyfakefs() + @staticmethod def create_metadata(): metadata = Dataset() @@ -93,3 +103,17 @@ def test_non_fatal_errors(self): self.assertEqual(1, len(error_dict)) errors = error_dict['test'] self.assertNotIn('fatal', errors) + + +class RealDicomFileValidatorTest(DicomFileValidatorTestBase): + + def test_that_pixeldata_is_read(self): + # regression test for #6 + rtdose_path = os.path.join(dicom_fixture_path(), 'rtdose.dcm') + error_dict = self.validator.validate(rtdose_path) + self.assertEqual(1, len(error_dict)) + results = error_dict[rtdose_path] + self.assertIn('RT Series', results) + self.assertIn('Tag (0008,1070) is missing', results['RT Series']) + # if PixelData is not read, RT Dose will show errors + self.assertNotIn('RT Dose', results) diff --git a/dcm_spec_tools/validator/tests/test_iod_validator.py b/dcm_spec_tools/validator/tests/test_iod_validator.py index 3887319..8410abb 100644 --- a/dcm_spec_tools/validator/tests/test_iod_validator.py +++ b/dcm_spec_tools/validator/tests/test_iod_validator.py @@ -63,7 +63,7 @@ def test_invalid_sop_class_id(self): @staticmethod def has_tag_error(messages, module_name, tag_id_string, error_kind): - if not module_name in messages: + if module_name not in messages: return False for message in messages[module_name]: if message.startswith( @@ -249,7 +249,7 @@ def test_presence_condition_met(self): 'PatientName': 'XXX', 'PatientID': 'ZZZ', 'PixelPaddingRangeLimit': '10', - 'PixelDataProviderURL': 'http://dataprovider' + 'PixelDataProviderURL': 'https://dataprovider' }) validator = self.validator(data_set) result = validator.validate() @@ -362,9 +362,10 @@ def test_condition_for_not_required_tag_no_cond_fulfilled(self): validator = self.validator(data_set) result = validator.validate() - self.assertTrue(self.has_tag_error(result, 'Cardiac Synchronization', - '(0018,9085)', - 'not allowed')) # Cardiac signal source + self.assertTrue(self.has_tag_error( + result, 'Cardiac Synchronization', + '(0018,9085)', + 'not allowed')) # Cardiac signal source def test_condition_for_not_required_tag_cond2_fulfilled_present(self): data_set = self.new_data_set({ @@ -379,9 +380,10 @@ def test_condition_for_not_required_tag_cond2_fulfilled_present(self): validator = self.validator(data_set) result = validator.validate() - self.assertFalse(self.has_tag_error(result, 'Cardiac Synchronization', - '(0018,9085)', - 'not allowed')) # Cardiac signal source + self.assertFalse(self.has_tag_error( + result, 'Cardiac Synchronization', + '(0018,9085)', + 'not allowed')) # Cardiac signal source def test_condition_for_not_required_tag_cond2_fulfilled_not_present(self): data_set = self.new_data_set({ @@ -395,9 +397,10 @@ def test_condition_for_not_required_tag_cond2_fulfilled_not_present(self): validator = self.validator(data_set) result = validator.validate() - self.assertFalse(self.has_tag_error(result, 'Cardiac Synchronization', - '(0018,9085)', - 'missing')) # Cardiac signal source + self.assertFalse(self.has_tag_error( + result, 'Cardiac Synchronization', + '(0018,9085)', + 'missing')) # Cardiac signal source if __name__ == '__main__': diff --git a/setup.py b/setup.py index 3a79a8a..1bd3fa2 100644 --- a/setup.py +++ b/setup.py @@ -40,6 +40,7 @@ "Programming Language :: Python :: 3.7", "Programming Language :: Python :: 3.8", "Programming Language :: Python :: 3.9", + "Programming Language :: Python :: 3.10", "Operating System :: POSIX :: Linux", 'Operating System :: MacOS', "Operating System :: Microsoft :: Windows", diff --git a/tox.ini b/tox.ini index 23360ba..f246d88 100644 --- a/tox.ini +++ b/tox.ini @@ -1,5 +1,5 @@ [tox] -envlist=py35,py36,py37 +envlist=py36,py37,py38,py39,py310 [testenv] deps = -rrequirements-dev.txt