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.

Sign up for GitHub

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

Jump to bottom

Make phase_cross_correlation return ndarray when disambiguate=True #7112

Merged
merged 9 commits into from
Sep 20, 2023
Merged

Make phase_cross_correlation return ndarray when disambiguate=True #7112

merged 9 commits into from
Sep 20, 2023

Conversation

decorouz
Copy link
Contributor

@decorouz decorouz commented Aug 29, 2023 •
edited by lagru

Description

ski.registration.phase_cross_correlation returns inconsistent results.
It returns a tuple when disambiguate is set to True but numpy array when set to False. Since there's no reason for a shift to sometimes be a tuple and sometimes a numpy array, this PR attempts to fix this bug #7083 by making the function return numpy array consistently.

Additionally, this gallery example returns a TypeError: can only concatenate tuple (not "int") to tuple when disambiguate is set to True on this line . This PR also fixed the error.

Checklist

Release note

Summarize the introduced changes in the code block below in one or a few sentences. The
summary will be included in the next release notes automatically:

Make `skimage.registration.phase_cross_correlation` consistently return an array even if
`disambiguate=True`.

@jarrodmillman jarrodmillman added the 🩹 type: Bug fix Fixes unexpected or incorrect behavior label Sep 7, 2023
mkcor
mkcor reviewed Sep 9, 2023
View reviewed changes
Copy link
Member

@mkcor mkcor left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this, @decorouz! I can't find the TypeError in the "Image Registration" gallery example though...

skimage/registration/tests/test_phase_cross_correlation.py Outdated Show resolved Hide resolved
skimage/registration/_phase_cross_correlation.py
@@ -172,11 +172,12 @@ def _disambiguate_shift(reference_image, moving_image, shift):
max_slice, positive_shift, negative_shift
):
real_shift_acc.append(pos_shift if sl.stop is None else neg_shift)
if not subpixel:
real_shift = tuple(map(int, real_shift_acc))
Copy link
Member

Choose a reason for hiding this comment

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

And since real_shift was supposed to be a 'tuple of float,' it's not clear why this mapping to integer was even performed...

Copy link
Member

Choose a reason for hiding this comment

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

I am also not seeing an immediate reason in #6617 why this mapping to int is performed. @jni do you remember?

skimage/registration/_phase_cross_correlation.py Show resolved Hide resolved
@decorouz @mkcor
Update skimage/registration/tests/test_phase_cross_correlation.py
cb2c773 
Co-authored-by: Marianne Corvellec <marianne.corvellec@ens-lyon.org>
@decorouz
Copy link
Contributor Author

decorouz commented Sep 9, 2023

Thanks for fixing this, @decorouz! I can't find the TypeError in the "Image Registration" gallery example though...

I have updated the PR description to better explain the TypeError. With current implementation, this gallery example result in a TypeError when disambiguate=True.

lagru
lagru requested changes Sep 10, 2023
View reviewed changes
Copy link
Member

@lagru lagru left a comment

Choose a reason for hiding this comment

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

Thanks for working on this. I think the test is not actually testing that the bug is fixed though. Otherwise, nothing major.

skimage/registration/_phase_cross_correlation.py Show resolved Hide resolved
skimage/registration/_phase_cross_correlation.py
@@ -172,11 +172,12 @@ def _disambiguate_shift(reference_image, moving_image, shift):
max_slice, positive_shift, negative_shift
):
real_shift_acc.append(pos_shift if sl.stop is None else neg_shift)
if not subpixel:
real_shift = tuple(map(int, real_shift_acc))
Copy link
Member

Choose a reason for hiding this comment

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

I am also not seeing an immediate reason in #6617 why this mapping to int is performed. @jni do you remember?

skimage/registration/tests/test_phase_cross_correlation.py Show resolved Hide resolved
m-albert
m-albert reviewed Sep 10, 2023
View reviewed changes
Copy link
Contributor

@m-albert m-albert left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this @decorouz :)

skimage/registration/tests/test_phase_cross_correlation.py Show resolved Hide resolved
@decorouz decorouz requested a review from lagru September 10, 2023 13:47
lagru
lagru approved these changes Sep 15, 2023
View reviewed changes
Copy link
Member

@lagru lagru left a comment

Choose a reason for hiding this comment

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

Looks good IMO.

mkcor
mkcor requested changes Sep 18, 2023
View reviewed changes
Copy link
Member

@mkcor mkcor left a comment

Choose a reason for hiding this comment

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

Yes, I think that's what I meant. Though, right now we don't need to append

shift = np.asarray(shift)

because this function already returns an array with

return np.array(real_shift_acc)

@lagru well, it's EITHER/OR, of course (not both). By 'right now,' I guess that you mean 'with this PR,' since only this PR is adding return np.array(real_shift_acc) (but this PR isn't merged yet... For me, 'right now' typically means 'on current main').

So,

  • EITHER this PR keeps return np.array(real_shift_acc) but then line 145 needs to be updated
-real_shift : sequence of floats
+real_shift : ndarray
  • OR this PR updates the code within phase_cross_correlation as discussed.

@mkcor @lagru
Update skimage/registration/_phase_cross_correlation.py
c940795 
Co-authored-by: Lars Grüter <lagru+github@mailbox.org>
@pep8speaks
Copy link

Hello @decorouz! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 256:1: E302 expected 2 blank lines, found 1
Line 266:13: E126 continuation line over-indented for hanging indent

@lagru
Copy link
Member

lagru commented Sep 20, 2023

All CI failures look related to #7140. As the test suite itself is passing fine, I am going ahead and merge this.

@lagru lagru merged commit 3cd5270 into scikit-image:main Sep 20, 2023
9 of 20 checks passed
@lagru
Copy link
Member

lagru commented Sep 20, 2023

Thanks everyone!

@stefanv stefanv added this to the 0.22 milestone Sep 20, 2023
@lagru lagru mentioned this pull request Sep 20, 2023
Closed
@mkcor mkcor mentioned this pull request Sep 20, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mkcor mkcor mkcor approved these changes

@m-albert m-albert m-albert left review comments

@lagru lagru lagru approved these changes

Assignees
No one assigned
Labels
🩹 type: Bug fix Fixes unexpected or incorrect behavior
Projects
None yet
Milestone
0.22
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@decorouz @pep8speaks @lagru @mkcor @m-albert @stefanv @jarrodmillman