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: MIP image filter for DRR image calculation #579

Merged
merged 1 commit into from
Mar 13, 2024

Conversation

MichaelColonel
Copy link

MIPForwardProjectionImageFilter is derived from
JosephForwardProjectionImageFilter and computes maximum intensity step along the ray casting on the projection

Example with parallel geometry beam.

DRR image with JosephForwardProjectionImageFilter:
drrOutput

DRR image with MIPForwardProjectionImageFilter:
mipDrrOutput

@SimonRit
Copy link
Collaborator

SimonRit commented Feb 1, 2024

Thank you for your contribution. Have you tried to avoid copy pasting the original code of Joseph's projector? It should not be necessary if one uses a functor.

@MichaelColonel
Copy link
Author

I've tried, but couldn't make it. Mainly because the SumAlongRay functor is a tally, in case of MIP we need a step with the maximum intensity not the sum of the steps along a ray. IMHO.

@SimonRit
Copy link
Collaborator

SimonRit commented Feb 1, 2024

I see. I guess including the sum as a reference parameter of SumAlongRay and doing the sum in the functor would solve this?
This code also needs to be changed using lambdas in my opinion but that's a more ambitious PR.

@SimonRit
Copy link
Collaborator

SimonRit commented Feb 1, 2024

Or doing a new AccumulateAlongRay lambda?

@MichaelColonel
Copy link
Author

I guess including the sum as a reference parameter of SumAlongRay and doing the sum in the functor would solve this?

I will try to use your suggestion, and use sum as a reference parameter in functor.

@MichaelColonel
Copy link
Author

Functor is changed, so copy-pasted part is removed.

This code also needs to be changed using lambdas in my opinion but that's a more ambitious PR.

Do you suggest of using lambdas instead of functors?

@SimonRit
Copy link
Collaborator

SimonRit commented Feb 1, 2024

Functor is changed, so copy-pasted part is removed.

Thanks! Looks better.

Do you suggest of using lambdas instead of functors?

Yes, something like what's been done here for ITK unary image filters. It would make the code look nicer but I'm not sure it would be as efficient. To be tested...

@SimonRit
Copy link
Collaborator

SimonRit commented Feb 1, 2024

There are some style errors:
https://github.com/RTKConsortium/RTK/actions/runs/7742228424/job/21110871876?pr=579

I would also suggest to change name from MIPForwardProjectionImageFilter to MaximumIntensityProjectionImageFilter to follow ITK's guidelines to avoid abbreviations.

@MichaelColonel
Copy link
Author

There are some style errors:
https://github.com/RTKConsortium/RTK/actions/runs/7742228424/job/21110871876?pr=579

I would also suggest to change name from MIPForwardProjectionImageFilter to MaximumIntensityProjectionImageFilter to follow ITK's guidelines to avoid abbreviations.

I will fix these errors.

@MichaelColonel
Copy link
Author

Filter was renamed, and MIP filter was added to rtkforwardprojections application.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks and sorry for not getting back to you before. Can the MIP code be put in a separate include/MaximumIntensityProjectionImageFilter.h(xx) pair files?

Copy link
Author

Choose a reason for hiding this comment

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

Sure.

Copy link
Author

Choose a reason for hiding this comment

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

Done.

@SimonRit
Copy link
Collaborator

SimonRit commented Feb 23, 2024

I think the code is ready to be merged. It would be good to add a test and the wrapping. If you are not willing to do them, please rebase on master and squash all commits in one. I'll then merge and do them in another PR.

@MichaelColonel
Copy link
Author

MichaelColonel commented Feb 26, 2024

I can try. I'll let you know in case of problems.

@MichaelColonel
Copy link
Author

Is there a instruction how to test python wrapping?

@LucasGandel
Copy link
Collaborator

Is there a instruction how to test python wrapping?

Ideally you can add a test similar to rtkFirstReconstruction.py and enable it here. Then you need to build RTK with testing and python wrapping enabled to be able to run the test with ctest

@SimonRit
Copy link
Collaborator

Thanks @LucasGandel! I had forgotten about this.
@MichaelColonel If you just want to test that newly created wheels will contain the wrapping, wheels are automatically generated by the CI, you can download them as artifacts and test them locally. Currently, this PR does not compile the wheel https://github.com/RTKConsortium/RTK/pull/579/checks#step:5:2085

@MichaelColonel
Copy link
Author

MichaelColonel commented Mar 6, 2024

I have a question about writing a wrapper for MIP filter. I've taken as a basis the JosephForwardProjectionAttenuated wrapper, since both class are derived from JosephForwardProjection.

My wrapper is

set(WRAPPER_AUTO_INCLUDE_HEADERS OFF)
itk_wrap_named_class("rtk::Functor::MaximumIntensityAlongRay" "rtkFunctorMaximumIntensityAlongRay")
  foreach(t ${WRAP_ITK_REAL})
    itk_wrap_template("${ITKM_${t}}${ITKM_${t}}" "${ITKT_${t}}, ${ITKT_${t}}")
  endforeach()
itk_end_wrap_class()
itk_wrap_named_class("rtk::Functor::MaximumIntensityProjectedValueAccumulation" "rtkFunctorMaximumIntensityProjectedValueAccumulation")
  foreach(t ${WRAP_ITK_REAL})
    itk_wrap_template("${ITKM_${t}}${ITKM_${t}}" "${ITKT_${t}}, ${ITKT_${t}}")
  endforeach()
itk_end_wrap_class()
set(WRAPPER_AUTO_INCLUDE_HEADERS ON)

itk_wrap_class("rtk::JosephForwardProjectionImageFilter" POINTER)
  foreach(t ${WRAP_ITK_REAL})
    itk_wrap_template("I${ITKM_${t}}3I${ITKM_${t}}3SWM${ITKM_${t}}D${ITKM_${t}}IPC"
      "itk::Image<${ITKT_${t}}, 3>, itk::Image< ${ITKT_${t}}, 3>, rtk::Functor::InterpolationWeightMultiplication<${ITKT_${t}}, ${ITKT_${t}}, ${ITKT_${t}}>, rtk::Functor::MaximumIntensityProjectedValueAccumulation<${ITKT_${t}}, ${ITKT_${t}}>, rtk::Functor::MaximumIntensityAlongRay<${ITKT_${t}}, ${ITKT_${t}}>")
  endforeach()
itk_end_wrap_class()

itk_wrap_class("rtk::MaximumIntensityProjectionImageFilter" POINTER)
  foreach(t ${WRAP_ITK_REAL})
    itk_wrap_template("I${ITKM_${t}}3I${ITKM_${t}}3SWM${ITKM_${t}}D${ITKM_${t}}IPC"
      "itk::Image<${ITKT_${t}}, 3>, itk::Image< ${ITKT_${t}}, 3>, rtk::Functor::InterpolationWeightMultiplication<${ITKT_${t}}, ${ITKT_${t}}, ${ITKT_${t}}>, rtk::Functor::MaximumIntensityProjectedValueAccumulation<${ITKT_${t}}, ${ITKT_${t}}>, rtk::Functor::MaximumIntensityAlongRay<${ITKT_${t}}, ${ITKT_${t}}>")
  endforeach()
itk_end_wrap_class()

itk_wrap_class("rtk::MaximumIntensityProjectionImageFilter" POINTER)
  foreach(t ${WRAP_ITK_REAL})
    itk_wrap_template("I${ITKM_${t}}3I${ITKM_${t}}3" "itk::Image<${ITKT_${t}}, 3>, itk::Image<${ITKT_${t}}, 3>")
  endforeach()
itk_end_wrap_class()

During the compilation there are multiple warning messages like below, as a result python packages can't be created.

rtkJosephForwardProjectionImageFilterIF3IF3SWMFDFIPC_Pointer in /work/ITK-cp39-cp39-manylinux_2_28_x64/Wrapping/Typedefs/rtkMaximumIntensityProjectionImageFilter.idx is already defined in /work/ITK-cp39-cp39-manylinux_2_28_x64/Wrapping/Typedefs/rtkJosephForwardAttenuatedProjectionImageFilter.idx.

Do you have a tutorial of some kind, how to write a wrapper for derived classes?

@SimonRit
Copy link
Collaborator

SimonRit commented Mar 6, 2024

The available tutorial is the ITK software guide section 9.5. The warning indicate that you already have an instance with the same name so you need to change the first paramter of the itk_wrap_template macro.

@MichaelColonel
Copy link
Author

It looks like compilation with python and tests are OK! Should i squash the commits into one?

@SimonRit
Copy link
Collaborator

Yes please squash! I tested the wrapping and it works perfectly. Thanks!

The MaximumIntensityForwardProjectionImageFilter is derived from
JosephForwardProjectionImageFilter and computes maximum
intensity step along the ray casting on the projection.
@MichaelColonel
Copy link
Author

Squashed in one commit.

@SimonRit SimonRit merged commit 081e017 into RTKConsortium:master Mar 13, 2024
37 checks passed
@SimonRit
Copy link
Collaborator

Thanks for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants