CTK
CTK copied to clipboard
commontk
BUG: Convert lower case dicom tags
@pieper @cpinter
reference discussion:
https://discourse.slicer.org/t/dicom-database-update-is-requested-after-new-slicer-version-but-appears-to-be-impossible/35821/18
I'm not completely familiar with this specific part of codebase of the cktDICOMDatabase, so this PR was just a quick fix following the discussion on https://discourse.slicer.org/t/dicom-database-update-is-requested-after-new-slicer-version-but-appears-to-be-impossible/35821/18. The idea wasn't to merge it as-is, but rather to use it as a starting point (I will change the PR status to draft). Reference commit introducing the regression is: https://github.com/commontk/CTK/commit/84187713304e4ed5a457ee9758de1af4a22d8dbd
I agree with you that a basic method shouldn't break with the first change in a commit. This indicates that CTK doesn't have adequate unit tests for the DICOM tag methods, so I think we need to add them as well (in addition to cleaning, proper documentation, etc....). In fact we discovered this regression only after 5 months.
It might be faster for Steve to apply the feedback, since he's the original author of the commit. However, if you think we should raise the priority of this issue with my HIVE hours, I can step in to help.
This looks good to me, but I agree more tests would be very helpful. I'm not funded on any projects related to this at the moment so if you have time available yes, please jump in. 🙏
As a general rule I suggest we minimize the use of hex, either as strings or as literals in the code, since they are very error prone and hard to read and work with. Although in the tag cache itself we may not be able to avoid using the hex strings. In general we should always use the tag names as defined by the dictionary.
@pieper @lassoan I summarized the issue in https://github.com/commontk/CTK/issues/1204, so we can work on it as soon as we have dev time available for it.
Please let me know, if I need to do anything else for this PR (which fix only the regression).
@lassoan I am using this PR for the next round of enhs and fixes for the visual dicom browser. I have changed the title accordingly
@Punzo There are a couple of important fixes here. Could we merge this as is (and you can create a follow-up pull request with more updates)?
@Punzo There are a couple of important fixes here. Could we merge this as is (and you can create a follow-up pull request with more updates)?
Yes for me it's good to go. Next week I will create a new PR with more ENH
These look good to me, thank you.
I have opened a PR in SLicer for updating the CTK hash https://github.com/Slicer/Slicer/pull/7751