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

Enable a force write option for files with abnormal VRs #162

Open
ejcurtis opened this issue Jan 19, 2021 · 14 comments
Open

Enable a force write option for files with abnormal VRs #162

ejcurtis opened this issue Jan 19, 2021 · 14 comments

Comments

@ejcurtis
Copy link

Hi all, I am a developer on the Dicom Standard Browser. We are using dcmjs to view and edit .dcm files in the browser. At the moment, the only files we can do this with are those that pass all of the VR validation in this file. Some of the files we are manipulating have abnormal VR's but we would still like to be able to write them to a file. We were wondering if there could be an option to force the file to write, even if some fields do not match their VR.

For reference: One of the errors we got is shown below
VR error

In this instance, we would like to be able to force that field to write to the file even if the value exceeds its max characters.

I am tagging @johndgeise to give more context.

@johndgiese
Copy link

Hey @pieper, I hope you're doing well.

In addition to the ability to write an invalid file, we also were hoping validate dcmjs DicomDicts somehow. Can you confirm that neither of these features are present in dcmjs? Also, do you think one or both of these features would be appropriate to include within dcmjs? If so, we may put together a PR to add this functionality to the library.

@pieper
Copy link
Collaborator

pieper commented Jan 19, 2021

👋 @johndgiese and @ejcurtis - yes, I think if it's exposed in an options object with a name like allowInvalidVRLength that is false by default we should be fine. It's always a tough call to support non-standard formats but since you are just trying to work losslessly with existing fields it makes sense.

When you say validating DicomDicts do you mean something like dciodvfy? Yes, I think that would be very nice. Either as a part of dcmjs or as another package in dcmjs-org.

@johndgiese
Copy link

Thanks @pieper , we'll checkout the allowInvalidVRLength option. I was actually thinking of just basic VR validation, but something more sophisticated like http://dclunie.com/dicom3tools/dciodvfy.html would be even better. I think for our first version of our DICOM editor we'll skip validation. Perhaps if the feature is widely used we'll consider adding it later.

Thanks for the quick response.

@pieper
Copy link
Collaborator

pieper commented Jan 19, 2021

Sounds good @johndgiese. I've often thought we could do a json schema for this but never took it past the concept stage. Looking forward to seeing your editor someday.

@WoonchanCho
Copy link
Contributor

Sounds great, I also wanted dcmjs to have an option like "allowInvalidVRLength".
The DICOM standard keeps evolving and this makes issue sometimes. For example, The current DICOM standard allows TM type to contain 14 bytes maximum but it was 16 bytes maximum by 2017. This caused the "Value exceeds max length" error in the old DICOM file.

Thank you so much.

@ejcurtis
Copy link
Author

Thank you all for the quick responses! I am excited to continue to explore the allowInvalidVRLength option. I look forward to keeping in touch.

@pieper
Copy link
Collaborator

pieper commented Jan 20, 2021

Glad to see progress on this! It would be great if you could keep example instances that represent edge cases that need to be handled. So far we have this data repository and are hoping to be able to host a DICOMweb store instance where they can be made available for CI testing. We're trying to find a way to capture ad hoc gotcha cases so we have better interoperability across tools. We want to host this in a way that won't result in out of control hosting costs.

@ejcurtis
Copy link
Author

@pieper I would be happy to share our DICOM file that presented the error discussed. @johndgiese is this file able to be shared (i.e. has no PII) with this data repository?

@johndgiese
Copy link

@ejcurtis I'm pretty sure it doesn't, but I'd check in the editor as well as with Russell.

@ejcurtis
Copy link
Author

Hey @pieper, I was just working on uploading the CT file that caused the error we were discussing to dcmjs-org data, but I do not have access rights to upload a file. Would you like me to email you the file or grant me access to the repo?

@ejcurtis
Copy link
Author

Thanks @pieper, I pushed a pr for the new file in case you wanted to approve before committing it to master.

@WoonchanCho
Copy link
Contributor

Hi @ejcurtis and @johndgiese I just wanted to check how the development of "allowInvalidVRLength" option is going.
I recently see a lot of cases where we need to use the options in my own project. I just wanted to know a rough timeline when you will raise a PR for this feature. Feel free to tell me if there is anything I can help you on this too.
Thank you so much.

@johndgiese
Copy link

johndgiese commented Mar 4, 2021 via email

@WoonchanCho
Copy link
Contributor

@johndgiese thank you for your reply. I see, then let me work on this and push a PR first. You can improve or overhaul it when you have time on this.

pieper pushed a commit that referenced this issue Mar 4, 2021
…#186)

- add allowInvalidVRLength (false by default) in the write options
- add a test case for testing this option
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants