Skip to content

Support for fixed-length vectors in SetInput functions of two-steps spectral filters #688

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

Conversation

cyrilmory
Copy link
Contributor

Support for fixed-length vectors in SetInputMeasuredProjections and SetInputDecomposedProjections functions of Simplex and Forward spectral filters, with a limited number of materials and bins
Adds a test case in rtkDecomposeSpectralProjectionsTest to test this feature

Copy link
Collaborator

@SimonRit SimonRit left a comment

Choose a reason for hiding this comment

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

It's probably working (I'll check the wrappings by rebasing on master) but I have some questions I'd like to discuss before mergin.

@@ -83,16 +85,29 @@ class ITK_TEMPLATE_EXPORT SimplexSpectralProjectionsDecompositionImageFilter

/** Set/Get the input material-decomposed stack of projections (only used for initialization) */
void
SetInputDecomposedProjections(const DecomposedProjectionsType * DecomposedProjections);
SetInputDecomposedProjections(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't it work if you keep the original

void  SetInputDecomposedProjections(const DecomposedProjectionsType * DecomposedProjections);

separate for clarity?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should. I'll change it.

void
SetInputFixedVectorLengthDecomposedProjections(
const itk::Image<itk::Vector<DecomposedProjectionsDataType, VNumberOfMaterials>,
DecomposedProjectionsType::ImageDimension> * DecomposedProjections);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it accessible from the wrapping? I'll check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does not have to be: SetInputFixedVectorLengthDecomposedProjections is supposed to be accessed only by SetInputDecomposedProjections, once it has determined which vector length to use. The wrappings should only expose SetInputDecomposedProjections

{
this->SetInputFixedVectorLengthDecomposedProjections<5>(ptr5);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe a warning if we don't do anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@SimonRit SimonRit force-pushed the FixedVectorLengthSpectralInputsOverload branch from ef525c4 to 8a759ea Compare February 22, 2025 07:34
cyrilmory added a commit to cyrilmory/CyrilsRTK that referenced this pull request Feb 27, 2025
Separate SetInputDecomposedProjections and SetInputMeasuredProjections
into two parts, based on their input type (either itk::VectorImage, the
default input type, or itk::Image<itk::Vector<>>, the newly supported type)
as requested in RTKConsortium#688 (comment)
Add a warning when SetInputDecomposedProjections or SetInputMeasuredProjections
fails to cast the input to one of the supported types, as requested in
RTKConsortium#688 (comment)
cyrilmory added a commit to cyrilmory/CyrilsRTK that referenced this pull request Feb 27, 2025
Separate SetInputDecomposedProjections and SetInputMeasuredProjections
into two parts, based on their input type (either itk::VectorImage, the
default input type, or itk::Image<itk::Vector<>>, the newly supported type)
as requested in RTKConsortium#688 (comment)
Add a warning when SetInputDecomposedProjections or SetInputMeasuredProjections
fails to cast the input to one of the supported types, as requested in
RTKConsortium#688 (comment)
@cyrilmory cyrilmory force-pushed the FixedVectorLengthSpectralInputsOverload branch from 8af69d8 to 47d6551 Compare February 27, 2025 09:42
One-step and two-steps spectral filters currently use different
image types internally. To increase their interoperability, they
should also accept in input the type of image they don't use internally.
This commit modifies SetInputDecomposedProjections and
SetInputMeasuredProjections in rtkSpectralForwardModelImageFilter
so that they accept itk::Image<itk::Vector> images (with a limited
number of possible vector lengths).
It also adds a test case in rtkdecomposespectralprojectionstest to test
the modified SetInput functions
Modified the SetInput functions in
rtkSimplexSpectralProjectionsDecompositionImageFilter,
as was done in commit fc16320
for rtkSpectralForwardModelImageFilter.
Updated test case 3 in rtkDecomposeSpectralProjectionsTest
to test those new functions
Separate SetInputDecomposedProjections and SetInputMeasuredProjections
into two parts, based on their input type (either itk::VectorImage, the
default input type, or itk::Image<itk::Vector<>>, the newly supported type)
as requested in RTKConsortium#688 (comment)
Add a warning when SetInputDecomposedProjections or SetInputMeasuredProjections
fails to cast the input to one of the supported types, as requested in
RTKConsortium#688 (comment)
@SimonRit SimonRit force-pushed the FixedVectorLengthSpectralInputsOverload branch from 47d6551 to 28c3f3c Compare March 13, 2025 08:59
@SimonRit
Copy link
Collaborator

SimonRit commented Mar 17, 2025

Sorry for taking my time to check the PR. The two-step spectral example does not work anymore with the generated python package which is not what I expected. Do you know what could cause this backward incompatibility? The error message is:

WARNING: In /work/include/rtkSpectralForwardModelImageFilter.hxx, line 130
SpectralForwardModelImageFilter (0x5fa3cba3d570): The input does not match any of the supported types, and has been ignored

WARNING: In /work/include/rtkSpectralForwardModelImageFilter.hxx, line 229
SpectralForwardModelImageFilter (0x5fa3cba3d570): The input does not match any of the supported types, and has been ignored

Traceback (most recent call last):
  File "/home/srit/src/rtk/rtk/examples/Spectral/SpectralTwoStep.py", line 163, in <module>
    counts_forward = rtk.spectral_forward_model_image_filter(input_decomposed_projections=decomposed_ref,
                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/srit/miniconda3/envs/test_cyril/lib/python3.12/site-packages/itk/support/helpers.py", line 194, in image_filter_wrapper
    return image_filter(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/srit/miniconda3/envs/test_cyril/lib/python3.12/site-packages/itk/rtkSpectralForwardModelImageFilterPython.py", line 725, in spectral_forward_model_image_filter
    return instance.__internal_call__()
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/srit/miniconda3/envs/test_cyril/lib/python3.12/site-packages/itk/ITKCommonBasePython.py", line 3327, in __internal_call__
    self.UpdateLargestPossibleRegion()
  File "/home/srit/miniconda3/envs/test_cyril/lib/python3.12/site-packages/itk/ITKCommonBasePython.py", line 3115, in UpdateLargestPossibleRegion
    return _ITKCommonBasePython.itkProcessObject_UpdateLargestPossibleRegion(self)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
RuntimeError: /work/ITK-source/ITK/Modules/Core/Common/src/itkProcessObject.cxx:1339:
ITK ERROR: SpectralForwardModelImageFilter(0x5fa3cba3d570): Input Primary is required but not set.

@cyrilmory
Copy link
Contributor Author

The warning is supposed to be issued once the input has been dynamic_cast to all possible supported types, and none of the dynamic_cast has succeeded. I'll look into this

@cyrilmory cyrilmory closed this Mar 17, 2025
@cyrilmory cyrilmory deleted the FixedVectorLengthSpectralInputsOverload branch March 17, 2025 10:50
cyrilmory added a commit to cyrilmory/CyrilsRTK that referenced this pull request Mar 17, 2025
… supported type

Add a warning when SetInputDecomposedProjections or SetInputMeasuredProjections
fails to cast the input to one of the supported types, as requested in
RTKConsortium#688 (comment)
cyrilmory added a commit to cyrilmory/CyrilsRTK that referenced this pull request Mar 17, 2025
… supported type

Add a warning when SetInputDecomposedProjections or SetInputMeasuredProjections
fails to cast the input to one of the supported types, as requested in
RTKConsortium#688 (comment)
SimonRit pushed a commit that referenced this pull request Mar 17, 2025
… supported type

Add a warning when SetInputDecomposedProjections or SetInputMeasuredProjections
fails to cast the input to one of the supported types, as requested in
#688 (comment)
axel-grc pushed a commit to axel-grc/RTK that referenced this pull request May 15, 2025
… supported type

Add a warning when SetInputDecomposedProjections or SetInputMeasuredProjections
fails to cast the input to one of the supported types, as requested in
RTKConsortium#688 (comment)
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.

2 participants