Skip to content
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

Fix for #309 #310

Merged
merged 4 commits into from
Oct 9, 2024
Merged

Fix for #309 #310

merged 4 commits into from
Oct 9, 2024

Conversation

rhaxton
Copy link
Contributor

@rhaxton rhaxton commented Oct 4, 2024

Changed sr_document.dcm test case to have a LongCodeValue tag for Language of Content

Changed search_tree in find_content_items to try all three CodeValue tags (CodeValue, LongCodeValue, URNCodeValue)

@CPBridge
Copy link
Collaborator

CPBridge commented Oct 5, 2024

Hi @rhaxton, thanks for the PR. I think your code changes are good, except for the failing style checks: https://github.com/ImagingDataCommons/highdicom/actions/runs/11187492783/job/31122527504?pr=310

However, I have one nitpick... I notice that you have changed the attribute used for language code in the example SR document to Long Code Value but do not change the value (121049). However, to be 100% strict about this, that is an invalid usage because the standard states (part 3 chapter 8 section 8.1):

The Long Code Value (0008,0119) or URN Code Value (0008,0120) is only used for codes that exceed the 16 character size limit of Code Value (0008,0100). If the code value length exceeds 16, the Code Value (0008,0100) shall not be present. If the code value length is 16 characters or less, the Code Value (0008,0100) shall contain the code and neither Long Code Value (0008,0119) nor URN Code Value (0008,0120) shall be present.

I.e. you cannot use Long Code Value here because the value is < 16 characters. While this isn't relevant for the current PR under discussion, I think it is important that any test files we distribute are as correct as possible, as they will likely be used for tests for all sorts of other behaviour and also serve as exemplars on how to construct these files.

Can you think of other correct usage of Long Code Value that we could use here instead? Or alternatively we could revert the change to the file and create a tweaked version in memory during the test by setting it with some long string nonsense value.

Copy link
Collaborator

@CPBridge CPBridge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above comment with requested changes

@CPBridge CPBridge added the bug Something isn't working label Oct 5, 2024
@rhaxton
Copy link
Contributor Author

rhaxton commented Oct 8, 2024

I reverted the test case file and added two test cases that modify in memory. I reread the test file in each case to avoid modifying / testing / reverting the shared data, hopefully that is OK?

Copy link
Collaborator

@CPBridge CPBridge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks for your help @rhaxton

@CPBridge CPBridge merged commit edf89e6 into ImagingDataCommons:master Oct 9, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants