Skip to content

[pixeldata] Apply VOI LUT when rendering images#599

Merged
Enet4 merged 4 commits intoEnet4:masterfrom
abustany:voi-lut
Jun 21, 2025
Merged

[pixeldata] Apply VOI LUT when rendering images#599
Enet4 merged 4 commits intoEnet4:masterfrom
abustany:voi-lut

Conversation

@abustany
Copy link
Copy Markdown
Contributor

Fixes: #232

Tested on couple of images locally, this seems to do the job.

There are couple of items I'd like to address before considering this one ready:

  • add couple of unit tests
  • get your feedback @Enet4 on whether the data structures like VoiLut are in the right place...
  • maybe fix the naming, somehow "VOI LUT" can refer to lot of similar but different things 😬 This seems to be an issue with the standard already, but any feedback on how to make names clearer is welcome
  • do we want to make the API public already? Else we could gate the new structs behind pub(crate) for now

@abustany
Copy link
Copy Markdown
Contributor Author

woops didn't realize I had broken the gdcm feature, will fix

@feliwir
Copy link
Copy Markdown
Contributor

feliwir commented Nov 11, 2024

I think this will conflict with #598 i just created earlier today

@Enet4 Enet4 added enhancement A-lib Area: library C-pixeldata Crate: dicom-pixeldata labels Nov 11, 2024
@Enet4
Copy link
Copy Markdown
Owner

Enet4 commented Nov 11, 2024

This does look substantial, thank you for working on this!

  • add couple of unit tests

That would be greatly appreciated. I wonder if some of the DICOM test files available feature VOI LUT sequences. I also happen to have a test enhanced CT scan which I think features a VOI LUT sequence, but I need to check.

  • get your feedback @Enet4 on whether the data structures like VoiLut are in the right place...
    maybe fix the naming, somehow "VOI LUT" can refer to lot of similar but different things 😬 This seems to be an issue with the standard already, but any feedback on how to make names clearer is welcome

From the quick glance I found nothing egregiously wrong, but I will make a better review when I am able.

  • do we want to make the API public already? Else we could gate the new structs behind pub(crate) for now

In this case there is already some value in the library picking up VOI LUT sequences when the object uses that instead of window levels, so we can start with crate-level visibility and expose it later.

@Enet4
Copy link
Copy Markdown
Owner

Enet4 commented Nov 11, 2024

I think this will conflict with #598 i just created earlier today

If it helps create some coordination, we can decide an order of contribution merges so that we know which one should be rebased.

@abustany
Copy link
Copy Markdown
Contributor Author

I think this will conflict with #598 i just created earlier today

yup, very likely :-D but I'm happy for you to merge first, I checked your PR and the conflict resolving shouldn't be too hard to do.

Copy link
Copy Markdown
Owner

@Enet4 Enet4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you again for working on this. I will only leave a few comments inline. Overall this seems to be heading in the right direction. Please let me know when the pull request is ready for a more thorough test.

@abustany
Copy link
Copy Markdown
Contributor Author

abustany commented Dec 7, 2024

Thanks for the comments! I'll try to take a look next week. I unfortunately haven't found the time yet to finalize the tests - but that's still on my todo 😬

@Enet4
Copy link
Copy Markdown
Owner

Enet4 commented Feb 14, 2025

Hello again! Feel free to let me know if you need assistance reviving this pull request, or if you would like someone else to take over.

@abustany
Copy link
Copy Markdown
Contributor Author

Hello! Sorry about not picking that up - it’s the typical situation where we’ve been using this in production without issues for a while, so the urgency of properly wrapping up the PR went down… And free time has somehow been scarce. Anyway, I’m totally happy with someone else taking over this PR, this is public code really :) If that helps upstreaming the code faster, then everybody wins. Else I still have it on my backlog.

Copy link
Copy Markdown
Owner

@Enet4 Enet4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took the liberty of rebasing and applying my recommendations, but then I found something curious. Would appreciate your insight.

@abustany
Copy link
Copy Markdown
Contributor Author

I took the liberty of rebasing and applying my recommendations

that's perfect, thanks a lot!

@Enet4 Enet4 self-assigned this Mar 11, 2025
@Enet4 Enet4 marked this pull request as ready for review March 20, 2025 19:31
Copy link
Copy Markdown
Owner

@Enet4 Enet4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're good here! Thank you for the initiative and follow-up! 👍

@Enet4 Enet4 merged commit f2ea3f0 into Enet4:master Jun 21, 2025
5 checks passed
@abustany
Copy link
Copy Markdown
Contributor Author

Thank you so much for driving this one to the end!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-lib Area: library C-pixeldata Crate: dicom-pixeldata enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

VOI LUT Sequence support

3 participants