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

add EncapsulatedDocumentLength #271

Closed

Conversation

flicaflow
Copy link
Contributor

This PR adds the EncapsulatedDocumentLength tag which is needed to implement Encapsulated PDF IOD.

Note: I added the necessary value in the tag package manually. I tried to use the python script but that resulted in a broken package. Please advise if there is a better way doing it.

@suyashkumar
Copy link
Owner

suyashkumar commented Apr 20, 2023

Thanks for the contribution! This part of the library could definitely use some help haha. Ideally we would keep it so these tag definitions match exactly what the generate script produces, so I'll look into that if there's an issue (and maybe we can upgrade it a little).

Probably we should update the list of tags as well, or even better, maybe we can use the JSON parsed standard from Innolitics as the baseline (note the tag you're asking for is indeed included there).

Something that came to mind--do you think it would be helpful in the interim to have an "add custom tag" API so that users could add custom tag data to the global tagDict that would be used when parsing dicoms downstream in that program?

EDIT: this reminds me of #147 and draft #169 (thank you @bpeake-illuscio)! I will carve out some time in the next 2 weeks to circle back to those get a refined solution merged!

@flicaflow
Copy link
Contributor Author

I had basically the same thought just didn't wanted to make opinionated changes which are not mergable. I would basically do the following:

  • parse the tags from the innolitics json
  • replace the py script with a go script (why use python here) which runs with go generate. It is a rewrite anyway.

I was also thinking about the AddCustom tag API. That is not only important because of missing official tags but also because the concept of private tags exist. You can basically just invent tags with uneven group numbers. And that is important if you want to store proprietary data e.g. with the RawData IOD.

I can give the code generation a shot, no promises when this will happen and no hard feelings if you wanna do it yourself ;)

@suyashkumar
Copy link
Owner

Indeed. Note that private tags won't break parsing even in today's code, but having them in the tagDict is helpful for parsing implicit transfer syntax private tags more semantically (instead of treating them as raw elements).

On the code generation, that's in line with what we've been thinking! As I mentioned check out #147 and draft PR #169. #169 actually does quite a bit already (thanks to @bpeake-illuscio @peake100) including rewriting the codegen in Go and using the innolitics dump. My plan was to go through that PR, and use it as a starting point to get something mergable and probably collaborate with @bpeake-illuscio if they have time or add commits to that work myself to get it in shape to merge. It's possible we could use some help on this, and would be happy to reach out if so!

In the meantime, perhaps we can think about something like tag.RegisterCustom/tag.AddCustom? I suppose this will require knowledge of what custom tags one would like to register in advance of parsing a dicom, but worst case scenario it'll fall to the fallback of reading it as UnknownVR which defaults to readString, essentially holding onto the bytes as a string (I need to double check how that roundtrips also). It should also probably be easier to later try to interpret those bytes as other valueTypes in the future too.

@flicaflow
Copy link
Contributor Author

I close this in favor of #342

@flicaflow flicaflow closed this Nov 8, 2024
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

Successfully merging this pull request may close these issues.

2 participants