-
Notifications
You must be signed in to change notification settings - Fork 23
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
Consolidate interpolator #71
base: main
Are you sure you want to change the base?
Consolidate interpolator #71
Conversation
23cfe3f
to
5a6fddd
Compare
Rebased |
voxel sizes and triggering the case in which some arrival image voxels that contribute to gradient calculations have illegal values.
structure is all over the place. The new interpolator will only include neighbouring voxels in the calculation if they meet requirements defined in a user supplied function. The complication is that interpolators are used at a number of stages in this code. One point is directly in the cost function, the other is in the PhysicalCentralDifferenceImageFunction. There's currently no way to set the one in PhysicalCentralDifferenceImageFunction. It probably makes sense for the cost function interpolator to be used by PhysicalCentralDifferenceImageFunction.
Rather than setting a target offset time, which is tricky to set correctly unless the speed function is very flat (which is unrealistic). This patch ensures that a neighborhood around each target point is included as target points, ensuring that the neighbourhood arrival time is correctly computed. The patch also fixes a bug that meant that only one target point was included from each set (i.e extended seeds weren't working properly).
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.
Rebased on 164c017, see https://github.com/romangrothausmann/ITKMinimalPathExtraction/tree/ConsolidateInterpolator
@thewtex should I force push to your thewtex:ConsolidateInterpolator?
The space changes can really drive one mad, hope I did not break anything while solving the merge conflicts. @richardbeare what do you think?
@romangrothausmann great! Yes, please go ahead and force-push. |
…ction For: /home/matt/src/ITK/Modules/Core/ImageFunction/include/itkInterpolateImageFunction.h:150:3: note: unimplemented pure virtual method 'GetRadius' in 'LinearInterpolateSelectedNeighborsImageFunction' GetRadius is a pure virtual in ITK 5.
d7a17a6
to
1c4e7c1
Compare
Tested this with the same CLI and get the same results as with the implementation provided by @richardbeare for #61: |
@romangrothausmann awesome! @richardbeare would be wonderful to get your review. |
On the to-do list |
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 think you have covered everything I tried to add. I can see the commits relating to an old bug with extended seeds, ensuring that the arrival times of neighbours of target points are evaluated, automated estimation of stopping conditions as well as the special interpolator.
I can't remember experimenting with anything else, so I think it is good to go.
It looks like some test baselines need to be updated and other tests are not generating baselines. @romangrothausmann do all tests pass locally? |
Sorry, only tested with my CLIs and the expected results, I guess the tests need to be updated to new standards of ITK-5 as well. |
No description provided.