Repository navigation
Conversation
|
@ntustison, could you review this when you have time? It makes SyN, BSplineSyN and time-varying velocity field registration work with |
|
@greptileai review |
[Medium impact] The changes reach registration workflows and a shared gradient filter when global data release is enabled. Fix the metric's lost channel selection and the velocity interpolator's released input before merging. Findings
|
hjmjohnson
left a comment
There was a problem hiding this comment.
Thanks. For plain Image inputs the approach holds: a graft shares the buffer, so results do not change, and the test fails on main with the "must have the same size" error from the #7078 thread (reasoned from the code, not run).
Requesting changes. One item is blocking because it changes results even with the flag off, and Greptile flagged it too:
- Blocking:
GradientRecursiveGaussianImageFilternow always grafts its input, andImageAdaptor::Graft(itkImageAdaptor.hxx:67-76) does not copy the pixel accessor. AnNthElementImageAdaptororVectorImageToImageAdaptorinput then reads component 0. I confirmed this in the code. Details inline. - Should-fix: masked BSplineSyN still fails with the flag on. Its fixed and moving mask resamplers (around BSplineSyN.hxx:336) are not grafted, unlike SyN's four, and neither is the one at
itkTimeVaryingBSplineVelocityFieldImageRegistrationMethod.hxx:738. The test has no masked case, so it cannot catch this. - Should-fix: after
GenerateData,m_DerivativeFilterkeeps the last graft, and so keeps the input's buffer alive after the outer pipeline would have released it (inline). - Should-fix: the TimeVaryingBSplineVelocityField changes have no test, and the body says they may be incomplete. Please cover them or move them to a separate PR.
- Greptile's P2 on
GetBufferPointer()in the test holds (conventions.md: "Use iterators, not raw buffer access").
Non-blocking:
- Greptile's P1 on the velocity-field graft (TimeVaryingVelocityField.hxx:36-43) looks moot:
SetVelocityFieldwraps the field's buffer as the transform's parameters, so a field whose producer has not run is not a supported state. A one-line reply on that thread would close it. - "New + Graft" is written five ways, and
MakeGraftis a protected static that other modules cannot reuse. EXPECT_EQon 4,096-element vectors prints both vectors on failure, and GTest.cxx:54-55 hardcodes16.0and15.0next to the namedimageSize.- The PR body still ends with the "Generated with Claude Code" footer; the AI disclosure in
<details>is enough (non-blocking).
This review ran as /code-review max (Standards and Spec axes) on Opus 5.5. Once these are addressed I will re-check, then request a Greptile review and mark the PR ready.
d701177 to
3bf7382
Compare
|
Thanks @hjmjohnson. I amended the single commit; the new head is 3bf7382.
Non-blocking:
Tests (local, every test binary under
|
|
The time-varying B-spline velocity field part of the GlobalReleaseDataFlag fix is now #7096, stacked on this PR. It also needed changes in TimeVaryingBSplineVelocityFieldTransform and its parameters adaptor. |
3bf7382 to
9b7034c
Compare
|
@hjmjohnson, a heads-up on one change in 9b7034c that reaches outside the registration code. To fix Greptile's P1 on the metric grafts, With this change, the gradient filter's The velocity field interpolator P1 is fixed too: it points back at the velocity field after integration. These pass: the ImageAdaptors, ImageGradient, DisplacementField, Metricsv4, Smoothing and RegistrationMethodsv4 tests in my build (61/61), and the 7 release-flag GTests. #7096 is rebased on this as c968cc5, and its 9 tests pass. |
bf0d867 to
4209b16
Compare
4209b16 to
ccb7da4
Compare
hjmjohnson
left a comment
There was a problem hiding this comment.
Re-reviewed at ccb7da4 (the rebase on main). Every concern from my earlier review is addressed, and the Greptile threads check out against the code: the accessor copy for NthElementImageAdaptor, the mask grafts, the derivative filter releasing its last graft, the interpolator restore on both paths, and the iterator-based test. Thank you for the thorough rework.
Three new should-fix points are inline; one of them is a wrong-result case for VectorImageToImageAdaptor. Behavior claims are reasoned from the code, not run.
Nits that have no changed line to anchor on:
itkBSplineSyNImageRegistrationMethod.hxxlines 161 to 174 still pass ungrafted fields to the composers, where SyN's composers are grafted. Nothing reads the released field beforeSetDisplacementFieldreplaces it, so this is consistency only.- The tests for the
ImageToImageMetricv4andTimeVaryingVelocityFieldTransformfixes live in RegistrationMethodsv4, so a build without that module does not run them. - The commit body says "Results are unchanged"; that holds for registration, while
ImageAdaptor::Graftresults change on purpose. - The graft idiom is still written several ways (
MakeGraft, a lambda, and inline copies). Optional.
| // Now copy anything remaining that is needed | ||
| this->SetPixelContainer(const_cast<Self *>(imgData)->GetPixelContainer()); | ||
| // The accessor selects what the adaptor reads, for example the element of an NthElementImageAdaptor. | ||
| this->m_PixelAccessor = imgData->m_PixelAccessor; |
There was a problem hiding this comment.
should-fix. Copying only the accessor leaves a VectorImageToImageAdaptor graft inconsistent. Superclass::Graft calls ImageAdaptor::CopyInformation, which calls m_Image->CopyInformation(data) with the adaptor as source. The adaptor reports one component per pixel, so the graft's internal VectorImage gets vector length 1 while the copied accessor has the source's length. GetPixel at any index other than zero then computes the wrong buffer offset, and once the graft enters a pipeline UpdateOutputInformation calls UpdateAccessor, which resets the accessor's vector length to 1 as well. The gradient filter and metric grafts would then read wrong values for VectorImageToImageAdaptor inputs whenever the input is released.
Suggested fix: graft the internal image (m_Image->Graft(imgData->m_Image), which copies the vector length) before copying the accessor, or set the internal image's component count from the source's internal image.
There was a problem hiding this comment.
Thanks, confirmed and fixed in 1a28b6f, as you suggested. Graft now calls m_Image->Graft(imgData->m_Image) in place of SetPixelContainer, so the internal VectorImage gets the source's vector length, then copies the accessor. With the old code, the strengthened test reads 1, 101 and 102 where 201, 202 and 212 are expected.
| componentAdaptor->SetExtractComponentIndex(2); | ||
| auto componentGraft = ComponentAdaptorType::New(); | ||
| componentGraft->Graft(componentAdaptor); | ||
| EXPECT_EQ(componentGraft->GetPixel({}), 3.0F); |
There was a problem hiding this comment.
should-fix (test). Index {} has buffer offset 0 for any vector length, so this passes even when the graft's vector length is wrong. Please also read a nonzero index, for example componentGraft->GetPixel({ { 1, 1 } }), and read one value through an iterator after componentGraft->UpdateOutputInformation(). That test settles the comment on itkImageAdaptor.hxx.
There was a problem hiding this comment.
Done: the images now hold a different value at every pixel and component (100 c + 10 y + x). The test reads index (2, 1) of both grafts, then calls UpdateOutputInformation() on both and checks every pixel through an iterator. It fails with the previous Graft.
| } | ||
| } // namespace | ||
|
|
||
| TEST(SyNImageRegistrationMethod, GlobalReleaseDataFlagDoesNotChangeTheResult) |
There was a problem hiding this comment.
should-fix. The suite name SyNImageRegistrationMethod is already used by itkSyNImageRegistrationMethodGTest.cxx in the same GTest driver, and Documentation/AI/code-review-lessons.md asks for one unique suite name per .cxx file. That file also already has a GlobalReleaseDataFlagGuard class and a GlobalReleaseDataFlag test, so this file adds a second copy of the guard (line 118). Either move these SyN cases into the existing file and reuse its guard, or rename the suites here, for example SyNImageRegistrationMethodReleaseData.
There was a problem hiding this comment.
Done: every test in itkSyNImageRegistrationMethodReleaseDataGTest.cxx is now in one suite, GlobalReleaseDataFlag (for example GlobalReleaseDataFlag.SyNResultIsUnchanged), which no other file uses; #7096's two tests follow it. I kept the file's own guard, which is in its anonymous namespace, rather than merge the two files.
| { | ||
| // Now copy anything remaining that is needed | ||
| this->SetPixelContainer(const_cast<Self *>(imgData)->GetPixelContainer()); | ||
| // The accessor selects what the adaptor reads, for example the element of an NthElementImageAdaptor. |
There was a problem hiding this comment.
should-fix (history). This changes what Graft means for every ImageAdaptor subclass, in Core, inside a registration commit. It has its own test and the metric fix depends on it, so please split it into a first commit, for example BUG: Copy the pixel accessor in ImageAdaptor::Graft, with ImageAdaptor.GraftKeepsThePixelAccessor, and keep the registration commit second. Two small points for that commit: Graft changes the accessor without calling Modified(), and the doc block in itkImageAdaptor.h still ends with "then copies over the pixel container" without the accessor.
There was a problem hiding this comment.
Done: BUG: Copy the pixel accessor in ImageAdaptor::Graft (1a28b6f) is now the first commit, with ImageAdaptor.GraftKeepsThePixelAccessor, and the registration commit (494697b) is second. Graft calls Modified() after changing the accessor, and the doc block now says it grafts the adapted image and copies the accessor. The first commit passes its tests on its own: 24/24 for ImageAdaptors, ImageGradient and Smoothing.
ImageAdaptor::Graft copied the pixel container but not the pixel accessor, so a graft of an NthElementImageAdaptor or of a VectorImageToImageAdaptor read element or component 0, whatever the source selected. Graft the adapted image, which also carries a VectorImage's number of components, then copy the accessor. A GoogleTest reads both kinds of graft at every pixel, before and after UpdateOutputInformation.
…DataFlag With DataObject::GlobalReleaseDataFlag on, internal mini-pipelines released inputs that are read again: the virtual domain (a resampler reference image), the level images, the masks, the transform fields fed to the composers, inverters and integrators, and the metric images fed to the gradient filters. Feed those pipelines grafts, which share the buffers, and set resampler geometry with SetOutputParametersFromImage. The velocity field interpolator points back at the velocity field after integration. Registration results are unchanged. GoogleTests cover each case with the flag on, including adaptor inputs to the metric.
ccb7da4 to
494697b
Compare
|
Errors:
|
|
Thanks @hjmjohnson. Pushed 1a28b6f and 494697b on your rebase; #7096 is rebased on them (61a2cab). The three should-fixes are inline. The review-body nits:
Test results:
|
Make SyN, BSplineSyN and time-varying velocity field registration run with
DataObject::GlobalReleaseDataFlagon; onmainthey throw or segfault. Results are unchanged.Root cause
With the global flag, every internal mini-pipeline calls
ReleaseData()on its inputs afterUpdate(). Several of those inputs are read again later:SyNImageRegistrationMethodandBSplineSyNset the current-level virtual domain image as theResampleImageFilterreference image, which is a pipeline input. After the first resample, the metric's virtual image had no buffer ("Virtual domain and moving transform displacement field must have the same size").m_FixedSmoothImages,m_MovingSmoothImages) and the mask images, which every iteration reads again.ImageRegistrationMethodv4released the user's fixed and moving images and the virtual domain through its per-level shrink and smoothing filters.InvertDisplacementFieldImageFilterreleased its own input and output through its internal composer.TimeVaryingVelocityFieldTransform::IntegrateVelocityFieldreleased the transform's velocity field through the integrators.ImageToImageMetricv4released the metric's fixed and moving images through the gradient filters, andGradientRecursiveGaussianImageFilterreleased its own input after the first of its per-dimension passes.Fix
ImageRegistrationMethodv4gets a protectedMakeGraft()helper for the registration methods.SetOutputParametersFromImage(virtualDomainImage)instead of a reference image input, which gives the same output region and geometry.GradientRecursiveGaussianImageFiltergrafts its input once per pass, only when the input would be released. After the passes the filter drops the last graft.BUG: Copy the pixel accessor in ImageAdaptor::Graft:Graftnow grafts the adapted image, which carries aVectorImage's number of components, and copies the pixel accessor. Without it, a graft of anNthElementImageAdaptororVectorImageToImageAdaptorread element 0, so the metric and gradient-filter grafts of an adaptor input used the wrong channel. The registration commit depends on it.TimeVaryingVelocityFieldTransform::IntegrateVelocityFieldpoints the shared velocity field interpolator back at the velocity field after integrating, also on exceptions. The integrators had left it on their released input graft.ShouldIReleaseData()is true, so an unchanged image still does not trigger a new gradient computation.Test results
itkSyNImageRegistrationMethodReleaseDataGTest.cxx: a two-level 2-D registration withMeanSquaresImageToImageMetricv4(default gradient filters) for SyN and BSplineSyN (each with and without fixed and moving masks) and forTimeVaryingVelocityFieldImageRegistrationMethodv4. It requires the displacement and inverse displacement fields with the flag on duringUpdate()to equal exactly those of a run with the flag off. Onmainall 5 cases fail with exceptions. With this PR they pass at 1, 4, 8 and 16 threads. Each masked case fails when its mask grafts are removed.ITKGoogleTestis added to the module'sTEST_DEPENDS.itkGradientRecursiveGaussianImageFilterGTest.cxx: the gradient of anNthElementImageAdaptorinput equals exactly that of a scalar copy of the selected element, with the flag off and on. The flag-off case passes onmainand fails if the filter always grafts. The flag-on case fails onmainand fails without the accessor copy inImageAdaptor::Graft.ImageAdaptor.GraftKeepsThePixelAccessor: grafts of anNthElementImageAdaptorand of aVectorImageToImageAdaptorread the selected element at every pixel, before and afterUpdateOutputInformation(). It fails if only the pixel container and the accessor are copied, because the graft's vector length is then 1.itkSyNImageRegistrationMethodReleaseDataGTest.cxx, two more tests:GlobalReleaseDataFlag.MetricKeepsTheAdaptorChannel: the Mean Squares value and derivative for channel 1 of two-channel adaptor inputs are equal with the flag off and on. It fails without the accessor copy.GlobalReleaseDataFlag.VelocityFieldIntegrationLeavesTheInterpolatorOnTheField: after integration with the flag on, the interpolator's input is the velocity field, and evaluating it returns the field's value. It fails without the restore.cmpagainstmain: 18/18 identical.TimeVaryingBSplineVelocityFieldImageRegistrationMethodis unchanged here. Its transform needs the same treatment, so it will be a separate PR with its own test.AI assistance