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

Print a warning when assigning a non-existing DICOM Tag #1014

Closed
razorx89 opened this issue Dec 30, 2019 · 8 comments · Fixed by #1199
Closed

Print a warning when assigning a non-existing DICOM Tag #1014

razorx89 opened this issue Dec 30, 2019 · 8 comments · Fixed by #1199

Comments

@razorx89
Copy link

razorx89 commented Dec 30, 2019

Is your feature request related to a problem? Please describe.
This problem is related to #1013

import pydicom
ds = pydicom.Dataset()
ds.SegmentsOverlap = 'NO'  # (0x0062, 0x0013)
print(ds.to_json())
# {}

For non-existing tags in the DICOM dictionary, those assignments and reads don't work properly and you can only spot the difference when dumping the DICOM dataset.

Describe the solution you'd like
When the DICOM Tag lookup is not successful and the name is not another existing property/variable, then emit a warning or even an Exception should be thrown.

@mrbean-bremen
Copy link
Member

Hm, generally you can always add an attribute this way in Python, so forbidding this could be a bit unexpected, especially if working with classes derived from DataSet. Anyway this is a valid request, in my opinion. @darcymason, what do you think?

@razorx89
Copy link
Author

Maybe it could be restricted to names which are pascal-case, since those don't follow the PEP naming conventions.

@mrbean-bremen
Copy link
Member

Yes, that could indeed be a good compromise.

@scaramallion
Copy link
Member

scaramallion commented Jun 22, 2020

Maybe yet another config option. I don't really like the idea of forcing people to adapt their usual conventions. Although I have been bitten by this a couple of times...

@darcymason
Copy link
Member

I was just thinking of this again recently also, and occasionally get bitten momentarily.

I support the idea of warning on non-DICOM CamelCase attributes for now. I believe it is rare that people would intentionally use their own attributes, and even less likely they would use CamelCase if they did, and it would only warn for now, so is not stopping their code from operating normally. In pydicom 3.x, I think it should raise an exception rather than a warning and then maybe it should be able to be turned off.

How about this: a config option (allowed values of 'off', 'warn' or 'error') for non-dicom attributes CamelCase. It defaults now to 'warn' in pydicom 2.x but 'error' in 3.x. That gives people the option to turn it off in 3.x if it causes trouble with legacy code.

Or, maybe there could be a more strict value, to warn on any attribute setting that is not a name in a list: something like Python's __slots__ concept. But we could add that later.

One caveat: I think warnings are by default only issued once now; this one should probably warn on each occasion if possible, otherwise typos could be silently missed.

And ... side thought ... maybe we should have some option for people to turn on all future config defaults -- allows people to ensure code will work for next major pydicom version. Somewhat like Python's command-line options to turn all warnings into errors.

@SimonBiggs
Copy link
Contributor

SimonBiggs commented Jun 22, 2020

maybe we should have some option for people to turn on all future config defaults -- allows people to ensure code will work for next major pydicom version. Somewhat like Python's command-line options to turn all warnings into errors.

I really like that idea (and would use it immediately). Having an error raised for non-dicom tags is also a config option I would set to error in my own code if the option was available.

Actually... Past me liked the idea so much... I think I turned it on already:

https://github.com/pymedphys/pymedphys/blob/c3052444e00062d0b3e7a4ffdafe40a7c756d523/pymedphys/_dicom/create.py#L52

@darcymason
Copy link
Member

Another thought -- the Datasets.__contains__ should also check that the keyword is a valid DICOM keyword, otherwise it will definitely return False for a typo, or if the keyword is new since the pydicom dictionary was last updated.

@scaramallion
Copy link
Member

This has come in handy already:

../pydicom/dataset.py:1891: UserWarning: Camel case attribute 'PatientBirtDate' used which is not in the element keyword data dictionary

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants