Add inverse capability to KernelTransform #4529
Conversation
It would be nice to have a test that converts a small (e.g. 8x8) image A to B, using KernelImage, and then from B to C, using its inverse. Then |
| typename PointSetType::Pointer sourceLandmarks = PointSetType::New(); | ||
| typename PointSetType::Pointer targetLandmarks = PointSetType::New(); |
There was a problem hiding this comment.
Please consider using auto here. For the return type of Type::New() we have agreed that auto is appropriate: https://github.com/InsightSoftwareConsortium/ITKSoftwareGuide/blob/d1562dd6dbc929ba18fdf8ce8aa86e8337597d7e/SoftwareGuide/Latex/Appendices/CodingStyleGuide.tex#L1558-L1562
There was a problem hiding this comment.
const auto would be even nicer 😇
const auto sourceLandmarks = PointSetType::New();
const auto targetLandmarks = PointSetType::New();
| deepCopyLandmarks(this->GetSourceLandmarks(), sourceLandmarks); | ||
| deepCopyLandmarks(this->GetTargetLandmarks(), targetLandmarks); |
There was a problem hiding this comment.
Please consider directly copying the internal STL containers of the landmarks:
sourceLandmarks->GetPoints()->CastToSTLContainer() =
m_SourceLandmarks->GetPoints()->CastToSTLConstContainer();
targetLandmarks->GetPoints()->CastToSTLContainer() =
m_TargetLandmarks->GetPoints()->CastToSTLConstContainer();
|
Just as I was making a pull request, I realized that a test is needed. I don't really have time to make a meaningful test (aside from the use in my own program). I also see that is crashing, probably meaning the inversion method I added is not thread-safe. It would be good if someone took over this PR (I am eying you Niels 😄). Otherwise it is likely going to fester for a long time. |
| inverseTransform->SetSourceLandmarks(targetLandmarks); | ||
| inverseTransform->SetTargetLandmarks(sourceLandmarks); | ||
| inverseTransform->SetStiffness(this->GetStiffness()); |
There was a problem hiding this comment.
I think it's OK for a class to directly access its own data members, without calling Get and Set. So more like:
inverseTransform->m_SourceLandmarks = targetLandmarks;
inverseTransform->m_TargetLandmarks = sourceLandmarks;
inverseTransform->m_Stiffness = m_Stiffness;Right?
I can ask next week if it may be of interest to elastix. But otherwise this PR does not have a high priority to me. Does it have priority to you? If so, then I think you should try to make it ready, including a simple little test 🤷 Otherwise, just postpone until the summer 🌞 holidays 😸 |
|
It is not of high priority to me either. So it would be good if you added it to your TODO list for summer holidays 😄 |
PR Checklist