-
Notifications
You must be signed in to change notification settings - Fork 112
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 UN VR setting during naturalization/denaturalization #379
Merged
Merged
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
719da01
FIx UN vr denaturalization
andreidubov 5f18aeb
Update tests
andreidubov a5b22db
Revert saving UN vr in _vrMap
andreidubov 122a549
Add parsing from UN to dictionary vr
andreidubov 6568ba3
Remove parseUnknownVr flag
andreidubov 1a72d4a
FIx test for UN vr
andreidubov 24bb0d0
Fix space
andreidubov d38b9a0
Fix UN vr test formatting
andreidubov File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @pieper that this should be a separate step that should be configurable - maybe as an option going into the parse step, but also as a stand alone filter that works on any JSON DICOMweb metadata instance. That filter should then have some way to register named fixes and a way to disable fixes individually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please clarify this point, I want to make sure that I understood you correctly. Do you mean that I should add an option(flag) to
naturalizeDataset/denaturalizeDataset
and depending on this option(flag) apply or skip for this logic?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking you could have a special-purpose piece of code at the application level that is coded based on the unique information that it will be accepting data from a source (kpacs) that has known limitations in terms of generating standard dicom objects. I.e. in this case you know that certain tag values will have incorrect values. This special-purpose code should read the instance and apply whatever fixes are needed to make the instance standards-compliant and then send it to the dcmjs code.
We may want to provide a standard place for this kind of special-purpose code to happen. Perhaps in dcmjs-dimse there could be a hook to apply a standardizing transform to the incoming instance data. If there end up being very common ones they could be bundled as add-on packages (e.g. a set of transforms for kpacs, ones for non-standard microCT scanners, etc).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have access to this low-level data on the application level as the application provides C-Store service only thing we have from dcmjs-dimse is Dataset without any VR information. dcmjs-dimse relies on dcmjs while reading the Dataset. In this case, we don't know if something is wrong until we have read the data.
What you propose is rewriting parts of dcmjs and dcmjs-dimse that are receiving binary data over the network and naturalizing the dataset.
The other thing is that we don't have access to the KPacs dictionary and can't compare it to dcmjs Dictionary to find out which tags can have the wrong VRs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I understand if dcmjs-dimse is processing a c-store you should be able to access the instance as it is received. From there you can read it as pure dicom json model and you can look at each of the tags in the instance and see if they have the non-standard form that leads to the issue based on the dicom dictionary and your knowledge of the deficiencies with kpacs. (That is, if you see that a tag with a know standard VR has been set to UN by kpacs). If you see an issue, you can modify the instances and save them back before further processing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the application level in CStore we do not have DICOM json, we only have naturalized Dataset. At this level, there is no way how we could get the original VR for tag.
DICOM json object is present inside dsmjs-dmse in the buffer/file reading functions.
If we talk about saving the original UN VR and saving it to a file without getting an error (as suggested in PR). I think it’s impossible to do without changes in dcmjs. The denaturalizeDataset takes the VR based only on the value in the dictionary, which leads to a mismatch between the VR and the value. If we talk about converting UN VR value to VR from the dictionary, it looks like a more difficult task to handle all possible VRs. @PantelisGeorgiadis do you have any ideas how this issue could be solved at the dcmjs-dimse level?