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

Fix SIFT wrong octave indices + typo #6184

Merged
merged 2 commits into from
Jan 13, 2022
Merged

Conversation

faberno
Copy link
Contributor

@faberno faberno commented Jan 12, 2022

Fixes #6173

  • typo: changed sigmas to scales
  • bug: wrong octave_indices if one octave wouldn't have keypoints, but the next one would

Description

Fixes the bug discovered by @longjoke. Extremas are saved in the extrema_pos list as seperate arrays for each octave. By using np.concatenate([np.full(len(p), i) for i, p in enumerate(extrema_pos)]), an array with the corresponding octave index of each extrema is created. If no extremas are found in an octave, nothing is appended to extrema_pos. This can to lead to wrong octave indices in a case where an octave has no keypoints, but the following does.
It's fixed by appending an empty array to extrema_pos, if no extremas are found in an octave.

I'm not sure, if it's possible that this case occurs.

For reviewers

  • Check that the PR title is short, concise, and will make sense 1 year
    later.
  • Check that new functions are imported in corresponding __init__.py.
  • Check that new features, API changes, and deprecations are mentioned in
    doc/release/release_dev.rst.

 - typo: changed sigmas to scales
 - bug: wrong octave_indices if one octave wouldn't have keypoints, but the next one would
@alexdesiqueira
Copy link
Member

@faberno thank you for taking care of #6173! LGTM.

@alexdesiqueira alexdesiqueira added 🩹 type: Bug fix Fixes unexpected or incorrect behavior action: mrg+1 labels Jan 13, 2022
@faberno
Copy link
Contributor Author

faberno commented Jan 13, 2022

@alexdesiqueira I've also just added a small check to _compute_orientation if there are any keypoints in an octave, similarly as we have it in _compute_descriptor. This was not a bug, but the check can save some unnecessary computations

Copy link
Member

@rfezzani rfezzani left a comment

Choose a reason for hiding this comment

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

Excellent, thank you @faberno 😉

@rfezzani rfezzani merged commit d046f89 into scikit-image:main Jan 13, 2022
@grlee77
Copy link
Contributor

grlee77 commented Jan 13, 2022

@meeseeksdev backport to v0.19.x

meeseeksmachine pushed a commit to meeseeksmachine/scikit-image that referenced this pull request Jan 13, 2022
grlee77 added a commit that referenced this pull request Jan 14, 2022
…4-on-v0.19.x

Backport PR #6184 on branch v0.19.x (Fix SIFT wrong octave indices + typo)
@lagru lagru added this to the 0.19.2 milestone Oct 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🩹 type: Bug fix Fixes unexpected or incorrect behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SIFT wrong octave values for keypoints
5 participants