Error when checking whether code is contained in context group #1388
Comments
|
Issue occurs because Hip joint is in CIDs 4 and 4031 (which is an inclusion of an inclusion). The newest version looks fine so the data probably needs to be regenerated, but I don't know enough about SR to judge whether or not that's a good idea without any versioning info... |
The context groups represent value sets and it should be fine for a value to be in more than one set. |
An update can be problematic if for whatever reason the meanings in the standard change, which would result in different Python dictionary keys and potentially break existing code. SNOMED-CT codes have synonyms and a different code meaning could in principle be used for the same code value. For example, the SNOMED Browser lists the following synonyms for Hip joint:
@dclunie could you provide some insight the process for updating CIDs in Part 16 (in particular the FHIR JSON format). Is there a chance for meanings to change from one edition of the standard to another? We should probably improve the implementation in the generate_code_dicts.py script. For example, we could check whether a code already exists for a given coding scheme (using the |
|
Yes, code meanings are changed/corrected/rationalized occasionally. |
|
Thanks for confirming @dclunie |
I believe I wrote that code, and vaguely remember putting in
I'll try to get my head around the code logic again and see how the assertion could be safely removed. Likely is an easy fix. |
|
Hmm, would it make sense to decouple the SR tables from pydicom itself and have them as a separate package ( Alternatively we could release pydicom packaged with the newest version of the tables and make it possible to use older (or newer) versions by installing a specific version of the separate package. Could probably just make it so a Github Action runs weekly (?) and checks the DICOM docbook for changes and performs auto-releases. If we wanted to we could probably do the same thing with the regular data dictionaries as well |
That could help, but by itself doesn't handle all problems because it is the meaning of the standards themselves that changes. But the pinning possibility would give users some options. @hackermd, thoughts?
Hmmm, that actually sounds good. We can have an import check for a separate pydicom-dicom-dict (or whatever it is called) and if newer than the one with pydicom, import that dictionary. Then users could update to latest DICOM dictionary without waiting for a pydicom release to catch up. I don't see older as being necessary, changes are backwards compatible, with the exception that some currently 'retired' items might not have been in previous version, or (rarely) some typo has been fixed. Having said that, it would be easy to have a flag for 'use even if older version', so I'd support it. |
I would prefer if we'd keep the tables in the library. There are only a few editions of the standard per year and I am thus not very concerned about the standard and the library getting out of sync.
We need to ensure that the codes are fully backwards compatible, therefore the need to use an older version of the table should not arise. If a code gets retired in the underlying coding schemes, I would suggest exposing the new version (rather than the retired version), but making sure that |
|
@hackermd, I started looking at this again - the original point about the |
|
The problem is that the pydicom attribute name matches multiple codes and may do so (and in fact actually does) for codes in the same CID and same scheme. I would suggest that we add explicit custom attribute names for those duplicates, possibly by appending the code to the end of the attribute? A list of duplicated attributes for the recent DICOM version: I've fixed some of the duplicates by adding more rules during processing, all the above are "true" duplicates in the sense that they have the same meaning in current DICOM. |
|
Sorry all, I'd forgotten about cleaning this up before the 2.2 release (#1259).
Do we have agreement on this? If the user needs to know the code to add to the name, then perhaps they can just use that directly instead. As always, I'm no expert in SR use, so appreciate comments from those that are, and how it best makes sense to solve this. As I noted before, the |
I would not do that. The problem with context groups such as CID 501 is that these groups include multiple other groups and some codes may occur in multiple included groups. We should now be able to handle these duplicates relatively easily using the We already have duplicate code entries in the code dicts, which are semantically equivalent (i.e., have the same Code Value and Coding Scheme Designator and |
Yeah agreed, apparently I'd forgotten what I'd said earlier in the same comment... |
Right, the |
|
So I started looking into this, and in current master, the first post code lines above give a different error: >>> from pydicom.sr.codedict import codes
>>> codes.SCT.Lung in codes.cid4
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "c:\git\pydicom\pydicom\sr\codedict.py", line 196, in __contains__
return any([concept == code for concept in self.concepts.values()])
File "c:\git\pydicom\pydicom\sr\codedict.py", line 87, in __getattr__
raise AttributeError(
AttributeError: 'concepts' not found in CID 4I assume this relates to something done in the last few pulls (original error replicated when I checkout v2.1.2 tag). Does anyone recognize where this is coming from? ... if not, I'll dig deeper. |
|
I've looked into this a bit - as shown, the error is in It appears this is happening because in building But I'm not sure yet why this is happening only in recent code in pydicom. I think I'll leave this for now unless someone knows where the new behavior came in, and work on the v2.2 pre-release (#1259) at a point before this was introduced. |
|
Thanks @darcymason for looking into the issue. |
|
Looked into again while figuring out what commit to use for v2.2 release - this was introduced in 1546700. Simple, really, the name of the exception thrown was changed from AssertionError to AttributeError. So, nothing too mysterious, decided to go to latest commit for release. We can fix this issue after the release. |
|
Something else to do: Tables J-1 and -2 need to be included and retired codes handled properly |
|
Adding 2.4 milestone to consider this for the release - this has been somewhat forgotten, it seems... |
Describe the bug
For certain context groups, an
AssertionErroris raised when one tries to check whether a given code is contained in the context group.Expected behavior
When a check is performed using the
inoperator whether an instance ofpydicom.sr.coding.Codeis contained inpydicom.sr.codedict.codes.cid*, a Boolean value should be returned and no Exception should be raised.Steps To Reproduce
Your environment
The text was updated successfully, but these errors were encountered: