Skip to content

ENH: Add option to access index in point set registration#2385

Merged
dzenanz merged 1 commit intoInsightSoftwareConsortium:masterfrom
samuelgerber:PointSetToPointSetMetricv4-IndexFunctions
Mar 13, 2021
Merged

ENH: Add option to access index in point set registration#2385
dzenanz merged 1 commit intoInsightSoftwareConsortium:masterfrom
samuelgerber:PointSetToPointSetMetricv4-IndexFunctions

Conversation

@samuelgerber
Copy link
Copy Markdown
Contributor

@samuelgerber samuelgerber commented Mar 9, 2021

This changes adds the class PointSetToPointSetMetricWithIndexv4 to the Metricsv4 module.
This class contains the previous implementation of everything in PointSetToPointSetMetricv4 but changes the
pure virtual functions such that the index in addition to the point is passed to the functions.
This avoids using the fixed points locator to find the index in subclasses that need access to it (see for example https://github.com/samuelgerber/ITKThinShellDemons/).

The pure virtual functions GetLocalNeighborhoodXXX are changed to pass the PointIdentiferType index of the point as well.

The PointSetToPointSetMetricv4 inherits from PointSetToPointSetMetricWithIndexv4 and passes the functions above to the original ones, such as to remain unchanged for classes inheriting from PointSetToPointSetMetricv4.

Thus future point set to point set metrics have the choice of which method to implement by choosing to inherit from
PointSetToPointSetMetricv4 or PointSetToPointSetMetricWithIndexv4.

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)

@samuelgerber samuelgerber force-pushed the PointSetToPointSetMetricv4-IndexFunctions branch from e9dba3d to 4bc00ac Compare March 9, 2021 03:22
@samuelgerber

This comment has been minimized.

@samuelgerber samuelgerber force-pushed the PointSetToPointSetMetricv4-IndexFunctions branch from 4bc00ac to bdf9774 Compare March 9, 2021 14:05
Copy link
Copy Markdown
Member

@dzenanz dzenanz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PointSetToPointSetMetricv4 inherits from it, so WithIndex variant needs to be wrapped too.

@thewtex thewtex requested a review from ntustison March 9, 2021 20:00
@samuelgerber

This comment has been minimized.

@samuelgerber
Copy link
Copy Markdown
Contributor Author

This functionality could also be added by adding those methods to PointSetToPointSetMetricv4.h and have them by default call the versions without index.
A superclass seems like a cleaner solution though and does not require subclass requiring access to the index to override both options.

@samuelgerber samuelgerber force-pushed the PointSetToPointSetMetricv4-IndexFunctions branch from bdf9774 to e490838 Compare March 11, 2021 16:09
@dzenanz
Copy link
Copy Markdown
Member

dzenanz commented Mar 11, 2021

Maybe also rebase on current master, to resolve merge conflicts.

- Add superclass to PointSetToPointSetMetricv4 to permit index based
  access in GetLocalNeighborhodXXX methods.
- Fix python wrapping.
@samuelgerber samuelgerber force-pushed the PointSetToPointSetMetricv4-IndexFunctions branch from e490838 to 97b878c Compare March 11, 2021 19:40
Copy link
Copy Markdown
Member

@dzenanz dzenanz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this is ready for merging?

@thewtex
Copy link
Copy Markdown
Member

thewtex commented Mar 12, 2021

LGTM

@samuelgerber thanks for pushing this upstream.

@ntustison does this look good to you?

@samuelgerber
Copy link
Copy Markdown
Contributor Author

I assume this is ready for merging?

I'd like to have @ntustison , or someone else familiar with the v4 framework, input on this before merging.
This functionality is not absolutely necessary but helpful for subcalsses in two instances for me so far. It could also be added in different ways.

@ntustison
Copy link
Copy Markdown
Member

Hey all, thanks for the consideration. It looks fine to me but it would seem as though y'all are stretching the capabilities more than we have so I would tend to go with your judgement with respect to this pull request. Thanks for all the work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants