-
-
Notifications
You must be signed in to change notification settings - Fork 138
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
MetaElementGroupLength not found #53
Comments
Thanks for raising this @thavlik, I think it makes sense for us to make a fix here to account for DICOMs like this! I'll look deeper into this either this weekend or later next week when I'm back from vacation. The solution you indicated that PyDicom does seem workable, but it would be great to avoid rewinding if possible. I think that we might be able to get by without rewinding (which would be awesome) so long as the correct We'll want to implement a fix for the current version and also the s/1.0-rewrite rewrite/refactor that I'm working on to update/fix many things from upstream branches. Are you able to share an example DICOM? Thank you so much for raising this! |
That's a great idea! I'm unable to confirm the eventual existence of MetaElementGroupLength for each item, so it might be missing entirely. Maybe they are present - I haven't checked. Here is an example DICOM: EDIT: can confirm ID_0015ba0f2.dcm only has: |
@thavlik Thank you for the investigation and the sample dicom! To support unstandardized dicoms like this, it appears that we may indeed need to investigate a rewinding mechanism, or some sort of I think it will be a couple weeks before I have bandwidth to really dig into this. PRs are welcome so let me know if you might be interested in proposing a fix (no worries if not). Will keep this issue updated as I revisit and in mind for the 1.0 rewrite too! |
One possibility is to read until the first non-metadata tag (this much is unavoidable) and then store a pointer to said tag within internal state. Parsing the body then entails considering this initialTag if it is non-nil, and subsequently setting it to nil - resuming current behavior. Kinda hacky but it's all I've got. |
@suyashkumar I'm looking to implement this fix now. |
I added an example DCM and made some changes at thavlik@15f3120. The tests do not pass. Currently digging into:
|
Hey @thavlik thanks for taking a look at this! I'll need to look a little closer at the test failure on your branch to help debug. In terms of the general approach, off the top of my head I think it might be sufficient to read until the first non group Will need to double check that there isn't anything else special we need to consider when reading that extra non-metadata tag (I think it should be okay, but we need to look up and see if metadata tags have any interaction with reading some of the downstream non-metadata tags and how we might want to deal with it if so) |
My branch is, functionally, doing exactly this. It's unclear why the rest of the file fails to parse as normal. |
The dicoms provided by https://www.kaggle.com/c/rsna-intracranial-hemorrhage-detection are not standardized to have MetaElementGroupLength come first. The pydicom library, which the dataset was probably intended for use with, accounts for this by reading until the first non-0x0002 tag then rewinding.
Example error output with the RSNA dataset (error is returned from dicom.NewParserFromBytes()):
The text was updated successfully, but these errors were encountered: