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

KeyError Exception when stripping sequences #137

Closed
wetzelj opened this issue Jul 1, 2020 · 2 comments
Closed

KeyError Exception when stripping sequences #137

wetzelj opened this issue Jul 1, 2020 · 2 comments

Comments

@wetzelj
Copy link
Contributor

wetzelj commented Jul 1, 2020

When stripping sequences from DICOM headers, when a given sequence element exists in multiple locations in the header - or when sequences are nested, the del dicom[elem.tag] statement in tags.py produces a KeyError. In the CtBrain1.dcm sample file, this scenario can be seen with tag (0008, 1110) which occurs as a top-level sequence but then also occurs as a nested item within a sequence further down in the header.

I've created a test method which demonstrates this bug and implemented a two possible fixes in this commit. Let me know if which fix (if either) you prefer and if you'd like me to submit it as a pull request.

Hi @vsoch! I'm back! :)

@vsoch
Copy link
Member

vsoch commented Jul 1, 2020

hey @wetzelj welcome back!!

I think solution A (the one not commented out) looks great - if we check for None then I don't think we will hit that key error. I vote for A, and we can adjust if there is some bug report about it.

When you open the PR (or put directly in the docstring, that's probably better) please briefly explain the test (e.g., I think we'd want to make sure that a nested value is properly removed). And I suspect we will hit other little bugs with sequences, but I'm happy to work on them as they come - smaller, well scoped PRs are good! Looking forward to your PR! Don't forget to update the changelog and do a version bump :)

@wetzelj wetzelj mentioned this issue Jul 2, 2020
3 tasks
@wetzelj
Copy link
Contributor Author

wetzelj commented Jul 2, 2020

Closing... Fixed with PR #138

@wetzelj wetzelj closed this as completed Jul 2, 2020
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

2 participants