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

Make cross_correlate_masked public #6036

Closed

Conversation

grlee77
Copy link
Contributor

@grlee77 grlee77 commented Nov 15, 2021

Description

Our existing cross_correlate_masked function is used for masked rigid registration, but is not currently exported as part of the public API. It would be useful to downstream projects to have this part of the official API (see prior discussion with @CSSFrancis: #5573 (comment)). It already has pretty good docstrings and independent test cases, so I don't see it being a maintenance burden to do so.

I did propose renaming m1->mask1 and m2->mask2 here to make the names a bit more descriptive. Given that this wasn't officially public API previously, I did not introduce a deprecation cycle for this. Also, should we have this under skimage.registration where it is now or perhaps somewhere else like skimage.util? What do others think?

Checklist

  • Docstrings for all functions
  • Gallery example in ./doc/examples (new features only)
  • Benchmark in ./benchmarks, if your changes aren't covered by an
    existing benchmark
  • Unit tests
  • Clean style in the spirit of PEP8
  • Descriptive commit messages (see below)

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.

This function already has standalone tests in test_masked_phase_cross_correlation.py
@grlee77 grlee77 added the ⏩ type: Enhancement Improve existing features label Nov 15, 2021
@jni
Copy link
Member

jni commented Nov 15, 2021

@grlee77 I don't think it should be public, this was part of the original discussion involving this function: it's accessed through phase_cross_correlation with either mask not equal to None.

@grlee77
Copy link
Contributor Author

grlee77 commented Nov 15, 2021

@grlee77 I don't think it should be public, this was part of the original discussion involving this function: it's accessed through phase_cross_correlation with either mask not equal to None.

I agree that is currently how it is accessed, but I think the intent in pyxem project was to access it directly rather than through the phase_cross_correlation function. At least, the PR linked in the other issue appears to include a vendored version of cross_correlate_masked with minor modification:
https://github.com/pyxem/pyxem/pull/750/files#diff-a2b0010135d99a660cff574a20f0d8f523de59ed52ddb8d648dd97e8083f53f8R153

For precedent in other libraries, the DIPlib API is a case where they have a FindShift function that operates like our (unmasked) phase_cross_correlation function, but the actual computation of the correlation can also be called via a separate function, CrossCorrelationFT.

@CSSFrancis
Copy link

@grlee77 @jni I think that adding this to the public API could be good in many cases. While the phase_cross_correlation function is useful in the case where you are aligning images, the cross-correlation can give much more information about some image.

It can be used to identify similar features in an image, identify rotational symmetry in a polar image, identify the extent of some feature in higher dimensional images etc. It most of these examples the important information is the number of peaks in the correlation as well as their size, rather than the position of the most intense peak (the shift returned by the phase_cross_correlation).

While there are many cross-correlation functions available in other packages, this is one of the only functions that properly treats masked regions of the image. I'd argue that making this part of the public API also important as it advertises the function as proper treatment of masked images when preforming the cross-correlation.

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.

Since people have use cases for this function, I'm more than happy to make it public! I would keep it under skimage.registration where it is now, alongside phase_cross_correlation.

@grlee77
Copy link
Contributor Author

grlee77 commented Nov 16, 2021

It can be used to identify similar features in an image, identify rotational symmetry in a polar image, identify the extent of some feature in higher dimensional images etc. It most of these examples the important information is the number of peaks in the correlation as well as their size, rather than the position of the most intense peak (the shift returned by the phase_cross_correlation).

We do try to provide an online gallery example for functions added to the public API. Do you have a good idea for a concise example relying on public domain data that we could use for this?

@jni
Copy link
Member

jni commented Nov 17, 2021

Ah, apologies, I hadn't realised that this function returned the cross correlation directly, not the shift. But could we not simply add cross_correlate() as the public API, masked or not?

@grlee77
Copy link
Contributor Author

grlee77 commented Nov 17, 2021

But could we not simply add cross_correlate() as the public API, masked or not?

Yeah that would be better and could incorporate some of the things mentioned in #5573 (comment). @CSSFrancis, if you want to make a PR unifying in a single cross_correlate function for the public API that would be great! We could then just close this PR in preference to that one

@grlee77 grlee77 marked this pull request as draft November 17, 2021 17:47
@CSSFrancis
Copy link

Yeah that would be better and could incorporate some of the things mentioned in #5573 (comment). @CSSFrancis, if you want to make a PR unifying in a single cross_correlate function for the public API that would be great! We could then just close this PR in preference to that one

Sure! I can do that hopefully tomorrow or this weekend.

@stefanv
Copy link
Member

stefanv commented Feb 7, 2023

Has this been superseded by #6077?

@stefanv
Copy link
Member

stefanv commented Apr 4, 2023

Closing in preference of #6077.

@stefanv stefanv closed this Apr 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⏩ type: Enhancement Improve existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants