Skip to content

PERF: less strict heuristic for valid points check#49

Closed
stnava wants to merge 3 commits intoInsightSoftwareConsortium:masterfrom
stnava:master
Closed

PERF: less strict heuristic for valid points check#49
stnava wants to merge 3 commits intoInsightSoftwareConsortium:masterfrom
stnava:master

Conversation

@stnava
Copy link
Member

@stnava stnava commented Jun 2, 2018

The previous check was overly strict. For example, one may be using a small mask (e.g. hippocampus) to focus registration within a much larger image. The registration result can be valid in this case despite a relatively small number of points used. Another alternative to this request would be to remove this check entirely.

See the CONTRIBUTING guide. Specifically:

Start ITK commit messages with a standard prefix (and a space):

  • BUG: fix for runtime crash or incorrect result
  • COMP: compiler error or warning fix
  • DOC: documentation change
  • ENH: new functionality
  • PERF: performance improvement
  • STYLE: no logic impact (indentation, comments)
  • WIP: Work In Progress not ready for merge

Provide a short, meaningful message that describes the change you made.

When the PR is based on a single commit, the commit message is usually left as the PR message.

A reference to a related issue or pull request in your repository. You can automatically close a related issues using keywords

@mentions of the person or team responsible for reviewing proposed changes.

Thanks for contributing to ITK!

stnava added 2 commits June 2, 2018 19:45
The previous check was overly strict.   For example, one may be using a small mask (e.g. hippocampus) to focus registration within a much larger image.   The registration result can be valid in this case despite a relatively small number of points used.  Another alternative to this request would be to remove this check entirely.
I found some examples that work well with very few valid points at low resolution.  Thus, I think this check should only be enforced when there are zero valid points.
@blowekamp
Copy link
Member

I believe the ideal solution may be to have a "minimum overlap" parameter, in all the metrics.

It is quite a common error for the initial registration to have no overlay. It is also not uncommon for an optimization to go awry resulting in little to no overlap.

@thewtex
Copy link
Member

thewtex commented Jun 6, 2018

Thanks for contributing this improvement @stnava 👍

We are still polishing the GitHub CI and have not finished the transition to GitHub quite yet -- the testing may give false positives until then.

@stnava
Copy link
Member Author

stnava commented Jun 6, 2018

practically speaking, i think we are erring on the wrong side here - rejecting too many valid registration setups as invalid. we've had several complaints about this and users have ultimately gone on to modify their own ITK copy.

@thewtex
Copy link
Member

thewtex commented Jul 20, 2018

Pushed to Gerrit here:

http://review.source.kitware.com/#/c/23580/

@thewtex
Copy link
Member

thewtex commented Jul 31, 2018

This has been merged.

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.

3 participants