ENH: Add lookups into shared/per frame functional groups

[naterichman]

* Currently implemented and tested for rescale slope/intercept only

[Enet4]

Thank you for picking this up. I will just leave some comments inline for now.

[naterichman]

Thanks for the review. I will integrate the comments. I also went through and added changes to the DecodedPixelData and the ImagingProperties, etc. Let me integrate the changes you suggested, and fix the tests that I've broken, and then I'll ping you again for a rereview!

[naterichman]

Tests are passing and I added changes to the DecodedPixelData object as well, I think this is now ready to be not a draft.

[Enet4]

From what I understood, the decoded object's transform LUT properties were expanded into Vec<_> so that there would always be one for each frame, correct? There may be room for some optimizations on how we do this (if the same properties apply to all frames, we wouldn't need to allocate 1-item vectors), but they can be deferred to a later date.
Some more suggestions inline.

[Enet4]

If you need some help with the suggested changes or would prefer that I take over instead, please let me know.

[naterichman]

Couple of questions in general if you don't mind.

• I didn't catch any of the GDCM stuff because my rust-analyzer had gdcm feature turned off by default. I was able to work around that by adding gdcm to the default features in my pixeldata Cargo.toml, but that seems hacky, is there a better way to do that that you know of?
• Relating to the above, is there way to run the CI jobs locally before I push to make sure they will pass? I'm coming over from python and have used pre-commit quite a bit. I've also seen some projects using just. Wondering if you have anything in place that I'm missing or if it would be worth adding something like pre-commit or just. LMK!

[ingwinlu]

• I didn't catch any of the GDCM stuff because my rust-analyzer had gdcm feature turned off by default. I was able to work around that by adding gdcm to the default features in my pixeldata Cargo.toml, but that seems hacky, is there a better way to do that that you know of?

Thats how I did it the first time too ;)

rust-analyzer has a config flag for it.
rust-analyzer.cargo.features is exposed in vscode if you use that. I am sure anything that is using rust-analyzer would have a similar way of passing the value through.

[Enet4]

• I didn't catch any of the GDCM stuff because my rust-analyzer had gdcm feature turned off by default. I was able to work around that by adding gdcm to the default features in my pixeldata Cargo.toml, but that seems hacky, is there a better way to do that that you know of?

Switching back and forth can be done through cargo parameters (--features gdcm) or by adjusting rust_analyzer in the IDE (thanks @ingwinlu).
The two implementations are pretty much exclusive.

A step to build against GDCM is included in CI in order to prevent larger mistakes from happening because of this.

• Relating to the above, is there way to run the CI jobs locally before I push to make sure they will pass? I'm coming over from python and have used pre-commit quite a bit. I've also seen some projects using just. Wondering if you have anything in place that I'm missing or if it would be worth adding something like pre-commit or just. LMK!

There is act, but I often just run the tests which relate to the changes made (e.g. cargo test -p dicom-pixeldata if I only want to run the tests for pixeldata), which is much more convenient. From the experience that I had with pre-commit hooks, I feel that they can be too much a computational burden to impose onto contributors at this time. In any case, extensions to CONTRIBUTING.md with nice tips and recommendations would be good to have!

---

Just a few more recommendations inline.

[naterichman]

Thanks again for the great review comments!

[Enet4]

I sense that we might need to give more leeway to unexpected inputs. While testing this, I found a file which features per-frame functional groups, but did not have a VOI LUT Sequence (functional groups were used for other attributes). In this scenario I would just expect the program continuing with the assumption that VOI LUT Function was not explicitly provided, but instead it assumed that this attribute should have been present. There might be similar issues for the other attributes.

2024-02-02T15:13:14.513214Z ERROR dicom_toimage: failed to decode pixel data

Caused by this error:
  1: Value multiplicity of VOI LUT Function must match the number of frames. Expected `1`, found `0`

I'm not sure whether I'll be able to come up with a full anonymized DICOM file, but this partial dump should give you an idea of the problem. Basically, rescale attributes are available in the Pixel Value Transformation Sequence, but the whole VOI LUT is left unspecified. This should turn into a warning, as in the current behavior.

(0028,0002) SamplesPerPixel              US (1,  2 bytes): 1
(0028,0004) PhotometricInterpretation    CS (1, 12 bytes): "MONOCHROME2"
(0028,0008) NumberOfFrames               IS (1,  2 bytes): "1"
(0028,0010) Rows                         US (1,  2 bytes): 672
(0028,0011) Columns                      US (1,  2 bytes): 672
(0028,0100) BitsAllocated                US (1,  2 bytes): 16
(0028,0101) BitsStored                   US (1,  2 bytes): 12
(0028,0102) HighBit                      US (1,  2 bytes): 11
(0028,0103) PixelRepresentation          US (1,  2 bytes): 0
(5200,9229) SharedFunctionalGroupsSequence SQ (1 Item)
    (FFFE,E000) na Item
      (0020,9116) PlaneOrientationSequence     SQ (1 Item)
          (FFFE,E000) na Item
            (0020,0037) ImageOrientationPatient      DS (6, 94 bytes): [".9962408542633", "-.0274781603366", "-.0821528509259", "-.0036975343246", ".93400490283966", "-.3572410345077"]
          (FFFE,E00D) ItemDelimitationItem
      (FFFE,E0DD) SequenceDelimitationItem
      (0028,9110) PixelMeasuresSequence        SQ (1 Item)
          (FFFE,E000) na Item
            (0018,0088) SpacingBetweenSlices         DS (1,  4 bytes): "4.4"
            (0028,0030) PixelSpacing                 DS (2, 30 bytes): [".3571428656578", ".3571428656578"]
          (FFFE,E00D) ItemDelimitationItem
      (FFFE,E0DD) SequenceDelimitationItem
      (0028,9145) PixelValueTransformationSequence SQ (1 Item)
          (FFFE,E000) na Item
            (0028,1052) RescaleIntercept             DS (1,  2 bytes): "0"
            (0028,1053) RescaleSlope                 DS (1, 16 bytes): "1.21514041514041"
          (FFFE,E00D) ItemDelimitationItem
      (FFFE,E0DD) SequenceDelimitationItem
    (FFFE,E00D) ItemDelimitationItem
(FFFE,E0DD) SequenceDelimitationItem
(5200,9230) PerFrameFunctionalGroupsSequence SQ (1 Item)
    (FFFE,E000) na Item
      (0020,9113) PlanePositionSequence        SQ (1 Item)
          (FFFE,E000) na Item
            (0020,0032) ImagePositionPatient         DS (3, 50 bytes): ["-117.20093870169", "-142.11923001646", "-4.5131105119807"]
          (FFFE,E00D) ItemDelimitationItem
      (FFFE,E0DD) SequenceDelimitationItem
    (FFFE,E00D) ItemDelimitationItem
(FFFE,E0DD) SequenceDelimitationItem

And in retrospection, I'm starting to think that different cardinality mismatches (e.g. 1 vs 3 frames) should not be something preventing the software from producing an image, although it should at the very least be a warning. This distinction would probably best be handled at the Decoded pixel data API, so that eventually a strict option could be implemented.

If you are still in good position to fix this, I would be very grateful.

[naterichman]

HI @Enet4 I can definitely still work on this, might not be for a week or so though!

[naterichman]

@Enet4 I added what I interpreted from your message above, but I'm not sure those checks are in the optimal place. LMK your thoughts and if you think its good, I'll go ahead with cleaning it up and adding tests

[Enet4]

Oh, I had my review just about ready a week ago, but I must have forgotten to submit it.

[Enet4]

👍 Thank you very much, @naterichman.

[naterichman]

Thank you, glad to be officially contributing to this project and hope to continue to!

[Enet4]

Oops! There are still some changes to make in the gdcm implementation. Would you be able to work on this?

[Enet4]

Thank you, glad to be officially contributing to this project and hope to continue to!

I am sorry to report that that opportunity has arrived so soon! 😅

[naterichman]

Sure thing! Get Outlook for iOS<https://aka.ms/o0ukef>

…

________________________________ From: Eduardo Pinho ***@***.***> Sent: Saturday, March 16, 2024 11:46:34 AM To: Enet4/dicom-rs ***@***.***> Cc: Nate Richman ***@***.***>; Mention ***@***.***> Subject: Re: [Enet4/dicom-rs] ENH: Add lookups into shared/per frame functional groups (PR #437) @Enet4 commented on this pull request. Oops! There are still some changes to make in the gdcm implementation. Would you be able to work on this? — Reply to this email directly, view it on GitHub<#437 (review)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AQJYZFS3GKX6HAPJYDIUZQLYYRZOVAVCNFSM6AAAAAA7OWTRX2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSNBRGE2TMMRTGE>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[naterichman]

I got the tests working for gdcm now, but I'm getting an issue locally in the openjpeg-sys feature:

$ cargo test -p dicom-pixeldata --features openjpeg-sys
   Compiling jpeg2k v0.6.6
   Compiling dicom-parser v0.6.0 (/home/naterichman/Documents/rust/dicom-rs/parser)
   Compiling dicom-test-files v0.2.1
error[E0255]: the name `sys` is defined multiple times
  --> /home/naterichman/.cargo/registry/src/index.crates.io-6f17d22bba15001f/jpeg2k-0.6.6/src/lib.rs:36:1
   |
33 | pub(crate) use openjpeg_sys as sys;
   |                ------------------- previous import of the module `sys` here
...
36 | pub(crate) mod sys {
   | ^^^^^^^^^^^^^^^^^^ `sys` redefined here
   |
   = note: `sys` must be defined only once in the type namespace of this module
help: you can use `as` to change the binding name of the import
   |
33 | pub(crate) use openjpeg_sys as other_sys;
   |                ~~~~~~~~~~~~~~~~~~~~~~~~~

For more information about this error, try `rustc --explain E0255`.
error: could not compile `jpeg2k` (lib) due to 1 previous error
warning: build failed, waiting for other jobs to finish...

I'm also getting that on master though...

[Enet4]

Did you perchance add the Cargo feature "jpeg-sys" without also excluding the default feature "openjp2"?

You can also rebase the branch to remove openjp2 from being enabled by default.

[Enet4]

OK, it should be in order this time. 👍 Thank you again!