Skip to content
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

STYLE: itkTimeVaryingBSplineVelocityFieldImageRegistrationMethod.hxx #2722

Merged
merged 1 commit into from
Oct 11, 2021

Conversation

josempozo
Copy link
Contributor

Removed useless operation: The body of the if condition only generated in-scope local objects which get removed when out of scope.

PR Checklist

  • No API changes were made (or the changes have been approved)
  • No major design changes were made (or the changes have been approved)
  • Added test (or behavior not changed)
  • Updated API documentation (or API not changed)
  • Added license to new files (if any)
  • Added Python wrapping to new files (if any) as described in ITK Software Guide Section 9.5
  • Added ITK examples for all new major features (if any)

Refer to the ITK Software Guide for
further development details if necessary.

Removed useless operation: The body of the if condition only generated in-scope local objects which get removed when out of scope.
@github-actions github-actions bot added the area:Registration Issues affecting the Registration module label Sep 8, 2021
@dzenanz
Copy link
Member

dzenanz commented Sep 8, 2021

The body of the if condition has a side-effect: indirectly updating movingDisplacementFieldTransform. I don't know how important that is.

@josempozo
Copy link
Contributor Author

The body of the if condition has a side-effect: indirectly updating movingDisplacementFieldTransform. I don't know how important that is.

The only call is movingDisplacementFieldTransform->GetDisplacementField().
This is a const function defined by itkGetConstObjectMacro. For what I understand, it only returns the pointer const to the displacementField (an itk::Image). Can there be any side effect triggered by that?

@dzenanz
Copy link
Member

dzenanz commented Sep 8, 2021

That by itself will not do much, but fieldDuplicatorIdentity->Update() will propagate the update to all its inputs. That probably includes movingDisplacementFieldTransform. These are comments from looking at the code, not from trying anything. The CI seems to be succeeding, so that side effect was probably not important.

@josempozo josempozo changed the title Update itkTimeVaryingBSplineVelocityFieldImageRegistrationMethod.hxx STYLE: itkTimeVaryingBSplineVelocityFieldImageRegistrationMethod.hxx Sep 15, 2021
@dzenanz
Copy link
Member

dzenanz commented Sep 15, 2021

You need to update the commit to add a STYLE: prefix. The PR title doesn't have to contain the prefix, as it can have multiple commits of different kinds.

@thewtex
Copy link
Member

thewtex commented Sep 30, 2021

@ntustison any thoughts on functionality and purpose of the code proposed for removal?

@ntustison
Copy link
Member

Hey @thewtex, sorry I missed this. Currently in London for the marathon but I'll take a look when I get back.

@thewtex
Copy link
Member

thewtex commented Oct 4, 2021

@ntustison thanks! Have a great race!! 🏃‍♂️ 👟 🥇

@ntustison
Copy link
Member

Sorry for the delay. Yeah, I don't know the reason for the code in question. All I can think of is that it was not removed when I integrated the point-set functionality. Thanks for looking at this.

@dzenanz dzenanz merged commit 52477f3 into InsightSoftwareConsortium:master Oct 11, 2021
@josempozo josempozo deleted the patch-1 branch December 5, 2021 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:Registration Issues affecting the Registration module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants