search

commontk / CTK

A set of common support code for medical imaging, surgical navigation, and related purposes.
https://commontk.org
Apache License 2.0
827 stars 480 forks source link

ENH+BUG for the visual DICOM browser #1203

Closed Punzo closed 1 month ago

Punzo commented 2 months ago

@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

Punzo commented 2 months ago

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.

pieper commented 2 months ago

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.

Punzo commented 1 month ago

@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).

Punzo commented 1 month ago

@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

lassoan commented 1 month ago

@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 commented 1 month ago

@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

Punzo commented 1 month ago

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