-
-
Notifications
You must be signed in to change notification settings - Fork 471
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
[WIP] Generated pydicom/sr codes now append '__' to Python keyword names #1281
base: main
Are you sure you want to change the base?
Conversation
Hello @seandoyle! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2021-06-04 00:15:33 UTC |
I'm unclear what I should do about the failing unit tests because they appear to be caused by changes outside of the scope of this PR. The PEP8 issues are on generated code - also a little outside the scope here. Open to suggestions. |
The test fails because the code for "Tc99mTetrofosmin" has changed - I actually don't understand it. There are a lot of changes (I guess you used a different version of the standard), but I was under the impression that these codes are fixed. Maybe @hackermd can have a look at this. |
We're still working on this. CP 1850 has the details - there was a large number of coding changes from SNOMED-RT to SNOMED-CT. We'll respond with more details and a proposal. |
We learned that not only code meanings, but also code values may be subject to change. DICOM defers to the individual coding schemes on how codes are maintained. SNOMED sometimes retires codes (not sure what the procedures are). We have too things that complicate comparison of codes:
We already account for It seems we also need to account for |
I would suggest the following algorithm for updating:
@mrbean-bremen what do you think? |
@hackermd - I will have a look tomorrow evening (didn't get to it today), though this is something that @darcymason and @scaramallion will probably want to check, too. |
SR is outside my area of expertise, I'm happy to defer to those with more experience |
cc'ing @dclunie who pointed out Table J-1 and helped clarify questions regarding retired codes |
SR is really outside my area of expertise too - I wrote some of the source code in pydicom, but I don't have knowledge really of the SR codes and their meanings/retirements etc. But having read through @hackermd's proposals and looking at the tables linked, I agree with the principles, which it sounds like are the same as previously - basically convert to latest definitions, be flexible on reading in codes, but more strict on assigning codes to try to keep to non-retired ones, and in any case try to be explicit rather than silently accepting ambiguous situations.
This sounds a bit tricky, but again, in principle seems like the safest way. |
Sorry, didn't get to this yesterday. I agree with @darcymason - as far as I can see, this all makes sense. And I also have to admit that I'm no expert by far for SR - I have been using it a long time ago and haven't been following it's development since then. |
Yes, that's going to be a bit tricky, but I think it's necessary. We should guarantee that the attributes of |
Just checking in on this PR -- we have upcoming release - can we get this in for the release? |
I'm sorry - wasn't sure of of some of the steps and let this slip. I'll take a look tonight. If we follow @hackermd's recommendation then we should also change the unit tests to match the latest values. Don't know how many of these there will be. What is the timeframe for the upcoming release? |
We don't have fixed dates, but had planned May some time back ... I'm late getting around to it anyway. I'd be fine with around 2 weeks, at least for a release candidate. |
@seandoyle, I've just rediscovered this PR - my apologies that this slipped through the cracks. I somehow missed back then that you had submitted new commits. Would there be any changes needed now before it could be pulled in? |
It's been a while - but I need to review it. The problem as I remember it was that incorporating the changes as is might break backward compatibility because some of the code values had changed.
I would vote for following @hackermd's approach but I may ask some questions as I go along. |
There are two things that could pose problems for backwards compatibility:
To ensure a dictionary entry in pydicom doesn't change if the Code Meaning changes in the standard, I suggest creating a "reverse" lookup table from the value (Code Value) to the corresponding dictionary keyword (derived from the "previous" Code Meaning). When we then regenerate the dictionary from the standard documents, we can find and reuse the existing keyword for a given value. |
Describe the changes
generated_concept_dicts.py
was modified to add '__' to names which were also keywords in Python. New versions of the dictionary codes in python/sr/* were generated bypython ./source/generate_cids/generate_concept_dicts.py
This handles issue 1273.
Tasks
test_cid3111
. Perhaps code libraries have changed?