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

ENH: Support regularization transform hardening in DICOM Scalar plugin #7362

Merged
merged 1 commit into from Nov 11, 2023

Conversation

Punzo
Copy link
Contributor

@Punzo Punzo commented Nov 8, 2023

this PR adds:

  • Grouping frames by orientation with a 10^-6 tolerance in examineFiles in DICOM module.
  • Add harden transform option in the DICOM settings for load in DICOMScalarVolumePlugin.

@Punzo Punzo force-pushed the main branch 2 times, most recently from 994d98b to d533d5a Compare November 8, 2023 13:30
@Punzo Punzo changed the title ENH: Fix orientation check in examinFiles in DICOM module and Add harden transform option in the DICOM settings ENH: Fix orientation check in examineFiles in DICOM module and Add harden transform option in the DICOM settings Nov 8, 2023
Copy link
Member

@pieper pieper left a comment

Choose a reason for hiding this comment

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

Thanks for working on this 👍

Another stylistic comment is that the commit message is longer than recommended.

https://slicer.readthedocs.io/en/latest/developer_guide/contributing.html#how-to-write-commit-messages

@Punzo
Copy link
Contributor Author

Punzo commented Nov 8, 2023

Thanks for working on this 👍

Another stylistic comment is that the commit message is longer than recommended.

https://slicer.readthedocs.io/en/latest/developer_guide/contributing.html#how-to-write-commit-messages

Thanks for the review Steve. I have addressed all the comments.

@pieper @lassoan up to you to merge if you think it is ready.

Copy link
Member

@pieper pieper left a comment

Choose a reason for hiding this comment

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

Sorry for all the nitpicks : )

But the diffusion issue is important since I don't think the current code will work.

Modules/Scripted/DICOMPlugins/DICOMScalarVolumePlugin.py Outdated Show resolved Hide resolved
Modules/Scripted/DICOMPlugins/DICOMScalarVolumePlugin.py Outdated Show resolved Hide resolved
Modules/Scripted/DICOMPlugins/DICOMScalarVolumePlugin.py Outdated Show resolved Hide resolved
pieper
pieper previously approved these changes Nov 9, 2023
Copy link
Member

@pieper pieper left a comment

Choose a reason for hiding this comment

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

Ahhh, looks nice now : )

@Punzo
Copy link
Contributor Author

Punzo commented Nov 9, 2023

Ahhh, looks nice now : )

perfect, thanks for the review :)

@Punzo Punzo requested review from lassoan and jcfr November 9, 2023 20:11
@Punzo
Copy link
Contributor Author

Punzo commented Nov 9, 2023

@jcfr @lassoan @pieper thank you very much for the reviews and sorry for the noise/spam.

I have reverted the class implementation commits. Please have a look at the current commit 0d2ae26 and let me know if there is any another issue.

Copy link
Member

@jcfr jcfr left a comment

Choose a reason for hiding this comment

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

Suggested commit message:

ENH: Add DICOM grouping frames by orientation and add harden transform option

Update the ScalarVolume DICOM plugin adding:
* subseries grouping by orientation 
* harden transform option in the DICOM settings.

Modules/Scripted/DICOMPlugins/DICOMScalarVolumePlugin.py Outdated Show resolved Hide resolved
@Punzo
Copy link
Contributor Author

Punzo commented Nov 9, 2023

Suggested commit message:

ENH: Add DICOM grouping frames by orientation and add harden transform option

Update the ScalarVolume DICOM plugin adding:
* subseries grouping by orientation 
* harden transform option in the DICOM settings.

@jcfr ok, tomorrow I will squash and change the commit message, thanks!

regarding the eps vs self.epsilon, I don't have strong preferences. However the default value of self.epsilon is 0.01. While we need a default value of 1.e-6 for the orientation check. We could multiply self.epsilon for a factor in examineFiles. Up to you.

@jcfr
Copy link
Member

jcfr commented Nov 9, 2023

To improve on #7362 (review):

ENH: Support regularization transform hardening in DICOM Scalar plugin

Update the logic identifying sub-series (each loaded as independent
volume node) to support grouping by orientation and add harden a new
DICOM settings option to enable automatic hardening of the corresponding
regularization transform.

I think this better describe what was implemented and help understand why.

re: Documentation

Consider also updating:

- DICOMScalarVolumePlugin settings:
- You can choose what back-end library to use (currently GDCM, DCMTK, or GDCM with DCMTK fallback with the last option being the default. This is provided in case some data is unsupported by one library or the other.
- Acquisition geometry regularization option supports the creation of a nonlinear transform that corrects for things like missing slices or gantry tilt in the acquisition. See more information [here](https://github.com/Slicer/Slicer/commit/3328b81211cb2e9ae16a0b49097744171c8c71c0)
- Autoloading subseries by time is an option break up some 4D acquisitions into individual volume, but is optional since some volumes are also acquired in time unites and should not be split.

@jcfr
Copy link
Member

jcfr commented Nov 9, 2023

re: epsilon

If the epsilon used in DICOMUtils.getSortedImageFiles and the one introduced in the examineFile are indeed different, we should name the prefix the parameter like we have done for other epsilon (cornerEpsilon, zeroEpsilon, ...)

For context, see #7362 (comment)

cc: @lassoan @pieper

@pieper
Copy link
Member

pieper commented Nov 9, 2023

should name the prefix the parameter like we have done for other epsilon

Agreed!

@Punzo Punzo force-pushed the main branch 3 times, most recently from 0779fac to 7ba13d2 Compare November 10, 2023 11:52
@Punzo Punzo requested a review from jcfr November 10, 2023 11:52
@Punzo
Copy link
Contributor Author

Punzo commented Nov 10, 2023

@jcfr I have applied last round of feedback!

Copy link
Contributor

@lassoan lassoan 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 all for the reviews, this looks good to me. I just added a remark on improving comments.

Update the logic identifying sub-series (each loaded as independent
volume node) to support grouping by orientation and add harden a new
DICOM settings option to enable automatic hardening of the corresponding
regularization transform.

Co-authored-by: Jean-Christophe Fillion-Robin <jchris.fillionr@kitware.com>
@Punzo
Copy link
Contributor Author

Punzo commented Nov 10, 2023

Thank you all for the reviews, this looks good to me. I just added a remark on improving comments.

Thanks Andras for the feedback. comments applied!

Copy link
Member

@jcfr jcfr left a comment

Choose a reason for hiding this comment

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

Pending local testing is successful 🚀

@jcfr jcfr changed the title ENH: Fix orientation check in examineFiles in DICOM module and Add harden transform option in the DICOM settings ENH: Support regularization transform hardening in DICOM Scalar plugin Nov 10, 2023
@jcfr
Copy link
Member

jcfr commented Nov 10, 2023

@Punzo Could you confirm that you performed local testing using the latest version ?

@jcfr
Copy link
Member

jcfr commented Nov 10, 2023

Once this is integrated:

There are probably other posts that could be updated. I used the query harden regularization2

Footnotes

  1. https://discourse.slicer.org/tag/feature

  2. https://discourse.slicer.org/search?q=harden%20regularization%20order%3Alatest

@jcfr jcfr added Type: Enhancement Improvement to functionality Domain: DICOM labels Nov 11, 2023
@Punzo
Copy link
Contributor Author

Punzo commented Nov 11, 2023

@Punzo Could you confirm that you performed local testing using the latest version ?

@jcfr yes I tested locally with several DICOM datasets.

@Punzo
Copy link
Contributor Author

Punzo commented Nov 11, 2023

Once this is integrated:

There are probably other posts that could be updated. I used the query harden regularization2

Footnotes

  1. https://discourse.slicer.org/tag/feature
  2. https://discourse.slicer.org/search?q=harden%20regularization%20order%3Alatest

ok!

@jcfr jcfr merged commit 0ec870e into Slicer:main Nov 11, 2023
6 checks passed
@Punzo
Copy link
Contributor Author

Punzo commented Nov 13, 2023

Once this is integrated:

There are probably other posts that could be updated. I used the query harden regularization2

Footnotes

  1. https://discourse.slicer.org/tag/feature
  2. https://discourse.slicer.org/search?q=harden%20regularization%20order%3Alatest

Done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Domain: DICOM Type: Enhancement Improvement to functionality
Development

Successfully merging this pull request may close these issues.

None yet

4 participants