-
Notifications
You must be signed in to change notification settings - Fork 161
ENH: Lambda functions instead of functor classes in JosephForwardProjectionFilter #694
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
base: main
Are you sure you want to change the base?
Conversation
|
Thanks a lot for doing this. Could you please modify the existing projector instead of creating a new one? That would facilitate the review and this is what we want eventually. |
I will do that, but what to do with JosephForwardAttenuatedProjectionImageFilter class? |
|
I've modified existing classes for JosephForwardProjectionFilter. |
…ardProjectionFilter Lambdas were added to the classes: JosephForwardProjectionImageFilter, JosephForwardAttenuatedProjectionImageFilter, MaximumIntensityProjectionImageFilter. Python wrappers for classes above have been modified. Similar lambda functions can be added to back projection classes.
arobert01
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the great work! It looks really good to me. I made some comments, mostly related to style.
Regarding JosephAttenuatedForwardProjector, based on the tests I did, everything seems to work well, and I don’t see a better way to do it. Maybe @SimonRit will have additional comments before merging.
If there are no further comments, we can also implement these changes in the Joseph-based back projectors.
Thanks again!
| * | ||
| * \author Simon Rit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would remove the author part of the comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok.
| using InputPixelType = typename TInputImage::PixelType; | ||
| using OutputPixelType = typename TOutputImage::PixelType; | ||
| using CoordinateType = double; | ||
| using WeightCoordinateType = typename itk::PixelTraits<InputPixelType>::ValueType; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WeightCoordinateType doesn't seem to be used in this case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be removed.
|
|
||
| std::cout << "\n\n****** Compare two resulted images ******" << std::endl; | ||
| CheckImageQuality<OutputImageType>(jfp->GetOutput(), mipfp->GetOutput(), 0.001, 0.000001, 1.E+19); | ||
| std::cout << "\n\nImages are OK! " << std::endl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I would keep the same "Test PASSED! " sentence for consistency ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will fix that.
| /** \brief Function to multiply the interpolation weights with the projected | ||
| * volume values and attenuation map. | ||
| * | ||
| * \author Antoine Robert |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove author section in the comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok.
|
Thank you for your excellent contribution @MichaelColonel and your review @arobert01. This is very interesting work but I will include it in the next RTK major release. I am currently working on a minor release and will merge this one afterwards. Note that #629 will probably require additional work for this PR but I'll handle it. |
Implementation of lambdas instead of functor classes in rtkJosephForwardProjectionFilter.
Test example of MIP filter using this new class.
Possible solution of issue #645.