search

dcmjs-org / dcmjs

Javascript implementation of DICOM manipulation
https://dcmjs.netlify.com/
MIT License
292 stars 111 forks source link

Fix errors parsing binary tags #276

Closed richard-viney closed 2 years ago

richard-viney commented 2 years ago

Have run into a few bugs in the binary tag parsing on some DICOMs, and this PR contains fixes and test cases for those issues.

The two included DCM files are 13MB and 3MB - should these be hosted on GitHub releases like the other data is?

Details:

pieper commented 2 years ago

This looks good, but also maybe has some edge conditions that should be tested. @Punzo would you be able to test this one too with Slim or OHIF?

@richard-viney yes, the data files should not be in the commit, but added to the data repo.

Punzo commented 2 years ago

tested over OHIF and Slim on a couple of studies, no regression found

image image

pieper commented 2 years ago

Thank you for testing @Punzo 👍

@richard-viney can you adapt the tests and force-push without the big files?

richard-viney commented 2 years ago

Thanks, I've switched to downloading the files in the tests. I think the URLs are right but can't confirm until the data files are available in the other repo. The two data files can be downloaded here:

https://drive.google.com/file/d/1tQFRfY9PBtOeDPuAB1ZCb9TNLlWuV8Zx/view?usp=sharing

pieper commented 2 years ago

The data is now in a release asset here: https://github.com/dcmjs-org/data/releases/tag/binary-parsing-stessors

I think you do need to tweak the URLs a bit.

richard-viney commented 2 years ago

URLs updated and tests pass. I think there's a typo though, "stessors" should be "stressors".

Ouwen commented 2 years ago

@richard-viney we seem to be modifying the same files, shall I include your changes on top of #279 or vice versa?

richard-viney commented 2 years ago

@Ouwen I think it's up to the maintainers, though you're right we have fixed at least one of the same bugs to do with incorrect stream offsets! I'm keen to get this PR in because it fixes failures parsing valid DICOM files, and adds more tests to that end. Do the tests added in this PR pass when run on your branch?

richard-viney commented 2 years ago

After looking at rebasing this after the merge of #279 a few things came up.

Please see my comment there for details: https://github.com/dcmjs-org/dcmjs/pull/279#issuecomment-1159866553

This PR now:

richard-viney commented 2 years ago

I've updated the PR and put the additional binary file here: https://drive.google.com/file/d/1DXM5ChyzPwf1co3gTChbk0QcP55FfqBc/view?usp=sharing

Ouwen commented 2 years ago

@richard-viney thanks for going through the trouble of rebasing. The tests pass for me once the binary file is placed into my test folder.

I made a minor commit which puts the start/stop as an kwarg instead of arg. This way future options like Uint8 view only can live easy without cluttering the public API.

https://github.com/HeartLab/dcmjs/pull/2/commits/aea9c6fb312558d008b414a848b10395e51143f4

richard-viney commented 2 years ago

@Ouwen Thanks I've put that change onto this PR, and also done some other refactoring in BinaryRepresentation.readBytes() to simplify the code a bit further.

Tests are all passing (once the binary-tag.dcm file is in the data repo)

I think we're OK to merge then?

Ouwen commented 2 years ago

@richard-viney if possible could we keep the .map for frame/fragment parsing? The reason is that there are some advantages to treating frame parsing as a parallel map operation vs explicit for-loop.

For these new changes, would you kindly separate out the new commit changes on top of your previous PR commit for easier review? Referencing the previous commit history here: https://github.com/ouwen/dcmjs/tree/heart

richard-viney commented 2 years ago

Sure thing, I'll submit those further changes on a separate PR and we can discuss there. Have updated this PR accordingly. Thanks.

Ouwen commented 2 years ago

@richard-viney many thanks for going through the trouble. This looks good to me for merge

pieper commented 2 years ago

URLs updated and tests pass. I think there's a typo though, "stessors" should be "stressors".

Thanks, I fixed the typo, they are now here: https://github.com/dcmjs-org/data/releases/tag/binary-parsing-stessors

pieper commented 2 years ago

This release is also ready now.

https://github.com/dcmjs-org/data/releases/tag/binary-tag

@richard-viney I think if you just update the stessor/stressor url it should be good to go. It looks like I don't have permission to edit your PR code.

richard-viney commented 2 years ago

I've updated this PR to fix the stessors typo, but the correct release link to the data isn't working, i.e. the URL still needs the typo in it to work, not sure renaming the release was sufficient?

pieper commented 2 years ago

Good catch - I had renamed the release but it turns out I also needed to rename the tags. Can you try again?

richard-viney commented 2 years ago

Tests passed 🎉

github-actions[bot] commented 2 years ago

:tada: This PR is included in version 0.24.1 :tada:

The release is available on:

Your semantic-release bot :package::rocket: