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 reversing y-axis of diffraction patterns in template matching #946

Merged
merged 6 commits into from
Sep 25, 2023

Conversation

rbjorge
Copy link
Contributor

@rbjorge rbjorge commented Aug 9, 2023

What does this PR do?
This pull request seeks to fix the problem raised in issue #925. In short, diffraction patterns to be template matched currently have the y-axis reversed when template matching is performed (when they are converted to polar coordinates). This PR modifies _warp_polar_custom() to make the polar angle increase counter-clockwise when converting to polar coordinates. This also required changing plot_template_over_pattern().

The root cause of this issue, as far as I can see, are the two different coordinate system origin conventions used: top-left origin in hyperspy/pyxem and bottom-left in diffsims. I think both of these make sense in their respective contexts.

There are three failing test functions:

It's not surprising that these tests fail, so I think it is best that I get some initial feedback on this PR before I spend time modifying the tests.

To do

  • Fix failing tests
  • Document conventions
  • List in changelog

@hakonanes
Copy link
Member

Thanks for taking the time to troubleshoot this inconsistency, @rbjorge! I hope @din14970 finds time to comment.

Things left to do in this PR, as far as I can see:

  • Update tests
  • List this (breaking) fix in the changelog
  • Update any affected tutorials

@CSSFrancis
Copy link
Member

@rbjorge I don't see a reason why this can't be merged once we clean up the tests, but I'd like @din14970 to comment on it first!

Out of curiosity does this match the frame of reference for Kikuchipy? I think we should try to be as internally consistent as possible.

Let me know if you need any help adding tests, or fixing them!

@din14970
Copy link
Contributor

LGTM I suppose if tests are fixed

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@rbjorge
Copy link
Contributor Author

rbjorge commented Aug 18, 2023

I've now fixed the tests and added the notebook on conventions (not sure where to put the png file, so I put it in the same folder as the notebooks). See #925 for my answer to @CSSFrancis's question on Kikuchipy vs. pyxem conventions.

I don't think that the notebooks on readthedocs.io are affected by this PR (I don't see any template matching there). However, some of the notebooks in pyxem/pyxem-demos are affected. Mainly, since the diffraction patterns are no longer flipped, the value of mirrored_template will usually switch, as expected (same for signs in get_n_best_matches()). However, because of the "noisy" results of template matching, sometimes a different solution will be picked instead. When I checked this PR on a dataset with 192,000 experimental patterns, the value of mirrored_template was switched for 97-98 % of the patterns.

For the notebook 11 Accelerated orientation mapping with template matching.ipynb this means that it is now the mirrored correlations that are the highest. There are some small changes to the IPF maps, but when overlaying the best-matching template on the experimental pattern it still looks reasonable.

Also, since the value of the Euler angles are changed by this PR, user scripts that rely on the actual numerical values will be affected (for example this one perhaps).

Maybe this should somehow be included in the changelog entry?

@CSSFrancis CSSFrancis merged commit ab361d2 into pyxem:main Sep 25, 2023
@rbjorge
Copy link
Contributor Author

rbjorge commented Sep 27, 2023

Thanks for getting this merged, @CSSFrancis . I see now that one consequence of this fix is that datasets that were template matched before this change, will have the template flipped when using plot_template_over_pattern() with this fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Template matching Euler angle reference frame
4 participants