dcmjs icon indicating copy to clipboard operation
dcmjs copied to clipboard

Enable a force write option for files with abnormal VRs

Open ejcurtis opened this issue 4 years ago • 14 comments
trafficstars

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.

ejcurtis avatar Jan 19 '21 21:01 ejcurtis

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.

johndgiese avatar Jan 19 '21 22:01 johndgiese

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

pieper avatar Jan 19 '21 22:01 pieper

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.

johndgiese avatar Jan 19 '21 23:01 johndgiese

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.

pieper avatar Jan 19 '21 23:01 pieper

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.

WoonchanCho avatar Jan 20 '21 01:01 WoonchanCho

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

ejcurtis avatar Jan 20 '21 13:01 ejcurtis

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.

pieper avatar Jan 20 '21 13:01 pieper

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

ejcurtis avatar Jan 20 '21 15:01 ejcurtis

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

johndgiese avatar Jan 20 '21 15:01 johndgiese

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 avatar Jan 20 '21 20:01 ejcurtis

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

ejcurtis avatar Jan 20 '21 21:01 ejcurtis

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.

WoonchanCho avatar Mar 03 '21 22:03 WoonchanCho

We’re busy with other priorities at the moment. I’m not sure when we’ll get to this.

On Mar 3, 2021, at 5:29 PM, Woonchan Cho [email protected] wrote:

 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.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

johndgiese avatar Mar 04 '21 06:03 johndgiese

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

WoonchanCho avatar Mar 04 '21 11:03 WoonchanCho