Enet4 / dicom-rs

Rust implementation of the DICOM standard
https://dicom-rs.github.io
Apache License 2.0
403 stars 75 forks source link

ENH: Add lookups into shared/per frame functional groups #437

Closed naterichman closed 3 months ago

naterichman commented 7 months ago

* Currently implemented and tested for rescale slope/intercept only

naterichman commented 7 months ago

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 commented 7 months ago

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

ingwinlu commented 5 months ago
  • 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.

naterichman commented 5 months ago

Thanks again for the great review comments!

naterichman commented 5 months ago

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

naterichman commented 4 months ago

@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

naterichman commented 3 months ago

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

Enet4 commented 3 months ago

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! :sweat_smile:

naterichman commented 3 months ago

Sure thing!

Get Outlook for iOShttps://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 GitHubhttps://github.com/Enet4/dicom-rs/pull/437#pullrequestreview-1941156231, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AQJYZFS3GKX6HAPJYDIUZQLYYRZOVAVCNFSM6AAAAAA7OWTRX2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSNBRGE2TMMRTGE. You are receiving this because you were mentioned.Message ID: @.***>

naterichman commented 3 months ago

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 commented 3 months ago

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.