RTKConsortium / RTK

Reconstruction Toolkit
Apache License 2.0
246 stars 145 forks source link

ENH: MIP image filter for DRR image calculation #579

Closed MichaelColonel closed 8 months ago

MichaelColonel commented 10 months ago

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

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

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

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

Or doing a new AccumulateAlongRay lambda?

MichaelColonel commented 10 months ago

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

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

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

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

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

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

SimonRit commented 9 months ago

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

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

MichaelColonel commented 9 months ago

Is there a instruction how to test python wrapping?

LucasGandel commented 9 months ago

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

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

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

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

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

SimonRit commented 8 months ago

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

MichaelColonel commented 8 months ago

Squashed in one commit.

SimonRit commented 8 months ago

Thanks for your contribution!