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.

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

Support for python 3.12, pyproject.toml, minor naming cleanups #57

Merged
merged 15 commits into from
Jan 12, 2024

Conversation

smjoshiatglobus
Copy link
Contributor

This pull request brings in these changes:

  • Replaced setup.cfg and setup.py with pyproject.toml. See issue Strategy for Python version compatibility? #54 .
  • README.md changes
    • Fixed broken example code for anonymize. The API requires delete_private_tags argument,
    • Fixed broken example code for anonymize_dataset. The previous one was throwing exception from pydicom
    • Copied the examples to a new examples directory to ensure they work
    • Naming cleanup per PEP 8 style guide
  • anonymizer.py Changes
    • Replaced deprecated pkg_resources with importlib
    • Renamed argument to delete_private_tags to sync up with the same argument for anonymize_dataset and PEP 8 style guide

README.md Outdated
Comment on lines 193 to 219
# Create a list of tags object that should contains id, type and value
fields = [
{ # Replaced by Anonymized
'id': (0x0040, 0xA123),
'type': 'LO',
'value': 'Annie de la Fontaine',
},
{ # Replaced with empty value
'id': (0x0008, 0x0050),
'type': 'TM',
'value': 'bar',
},
{ # Deleted
'id': (0x0018, 0x4000),
'type': 'VR',
'value': 'foo',
}
]

# Create a readable dataset for pydicom
data = pydicom.Dataset()

# Add each field into the dataset
for field in fields:
data.add_new(field['id'], field['type'], field['value'])

anonymize_dataset(data)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did you delete this example ? This can be useful.
Why not moving it in the examples folder you created ?

Copy link
Contributor Author

@smjoshiatglobus smjoshiatglobus Nov 16, 2023

Choose a reason for hiding this comment

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

I tried this example and could not get it to work. I can fix the first warning about 'bar' not being valid for TM, but have no idea how to fix the error of missing file_meta for the dataset. I took the easier route of using an existing DICOM data file!

Here is the output:

C:\Users\smjoshi\.virtualenvs\examples-gxyY8XxM\Lib\site-packages\pydicom\valuerep.py:443: UserWarning: Invalid value for VR TM: 'bar'.
  warnings.warn(msg)
Traceback (most recent call last):
  File "C:\development\dicom-anonymizer\examples\anonymize_fields.py", line 33, in <module>
    main()
  File "C:\development\dicom-anonymizer\examples\anonymize_fields.py", line 30, in main
    anonymize_dataset(data)
  File "C:\development\dicom-anonymizer\dicomanonymizer\simpledicomanonymizer.py", line 436, in anonymize_dataset
    action(dataset.file_meta, tag)
           ^^^^^^^^^^^^^^^^^
  File "C:\Users\smjoshi\.virtualenvs\examples-gxyY8XxM\Lib\site-packages\pydicom\dataset.py", line 908, in __getattr__
    return object.__getattribute__(self, name)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'Dataset' object has no attribute 'file_meta'

Copy link
Collaborator

Choose a reason for hiding this comment

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

That should be fixed with #61

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! I will pull in the changes into my branch and that should clean up this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have restored the example in README.md (actually, I copy-pasted your code, so the white-space differences are showing up!). Please re-review.

pyproject.toml Outdated
]
description = "Program to anonymize dicom files with default and custom rules"
readme = "README.md"
requires-python = ">=3.10"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why restrict to 3.10 and higher ? I tested with Python 3.9 and 3.8 and it seems to work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was my confusion. Per importlib.metadata documentation, it was "provisional" until 3.10, so I thought it was not available for versions 3.8 and 3.9. Since these worked, I will update this minimum requirement to 3.8.

Thank you for testing!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in commit b3dc158

@pchoisel
Copy link
Collaborator

Thank you very much for your work !

@pchoisel pchoisel merged commit 1f16e34 into KitwareMedical:master Jan 12, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants