ENH+BUG for the visual DICOM browser

[Punzo]

No description provided.

[Punzo]

@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

[lassoan]

The change looks good, but this is a sneaky error that has many other manifestations if we are not being very careful. We need to make the code as safe as possible by design, and if not possible then by documentation.

The obvious fix would be to not store DICOM tags as strings, because those 2x2 bytes could be stored in an int and then we would not have string comparison issues and also the comparison and any conversions would be much faster. However, if we want to support private tags in the future then we need to store private creator string as well and then integer will not suffice. So, let's keep it a string and make sure it is used correctly everywhere and improve the documentation to reduce chances of errors creeping in again.

There should minimum number (preferably only one) for converting a tag to string and it should be used everywhere. Not just capitalization, but also zero-padding can be inconsistent, and if we add support for private tags then we want to change the code at only one place. Check where else tags are converted to strings and make sure this single implementation is used everywhere.

We should probably also normalize tag string formatting in all tagcache methods (e.g., before storage or comparison of a tag string, it should be normalized). Similarly to tag to string conversion, there should be only one normalization method imlementation and that should be used everywhere.

Finally, we should improve documentation:

• Improve tagcache documentation: describe how the tag string is formatted (we can refer to the conversion or normalization method) in the database, and in get/set methods documentation describe what input formatting is allowed and what formatting is used for the output.
• Improve dicomTagToString method documentation
• Add a descriptive commit message.

[Punzo]

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: 8418771

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]

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]

@pieper @lassoan I summarized the issue in #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]

@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]

@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]

@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

[lassoan]

These look good to me, thank you.

[Punzo]

These look good to me, thank you.

I have opened a PR in SLicer for updating the CTK hash Slicer/Slicer#7751