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

Change left/right symmetry in reduction of (mis)orientations to fundamental zone #442

Draft
wants to merge 23 commits into
base: develop
Choose a base branch
from

Conversation

hakonanes
Copy link
Member

@hakonanes hakonanes commented Apr 6, 2023

Description of the change

This PR changes the order of symmetry application in the reduction of misorientations to the misorientation fundamental zone from sym_A * m_AB * sym_B to sym_B * m_AB * sym_A. The new order is in line with the notion of the misorientation from o_A to o_B being given by m_AB = o_B * o_A^-1.

Note that the same algorithm is used to reduce orientations to the Rodrigues fundamental zone, in which case the two symmetries are the symmetry of the sample reference frame, sym_A = C1 and the symmetry of the crystal, say sym_B = Oh.

This change was motivated by several inconsistent results reported lately. This change fixes all these issues:

I had to update all three clustering tutorials. They give comparable results, although I'm unsure of how to solve the mirroring of the clustered Ti orientations in that tutorial. Any help on spotting an error would be appreciated.

The other major change is naming: map_into_symmetry_reduced_zone() -> reduce(), inspired by the attractive naming of scipy.spatial.transforms.Rotation.reduce(). I've deprecated the former method in favor of the new one, with removal in v0.13. Arguably not as easy to understand as the former name, but the notion of reducing a (mis)orientation to its fundamental zone is there. Using this word allows us also to add reduce as an intuitive parameter to various methods if we don't want to apply symmetry operations, instead of having a use_symmetry parameter (similar to MTEX' noSymmetry). We could then rename Miller.in_fundamental_region() to Miller.reduce() etc.

Testing this change required some other tools and changes, which are also included in this PR:

  • New class method Vector3d.get_path() to get vectors along the path between two or more vectors. Useful to plot a bounded region on the stereographic projection, or delineate a path between two vectors.
  • Doc example showing how to combine rotations, i.e. from left to right. The example is the one in section 4.2.2 in Rowenhorst et al. (2015).
  • Changed time of removal of convention parameter in to/from_euler() methods from v1.0 to v0.13, because I don't see v1.0 on the horizon.
  • Make stereographic polar grid use Matplotlib's grid.alpha config value.
  • Restructure some tests for clarity.

Progress of the PR

Minimal example of the bug fix or new feature

I suggest to inspect the clustering tutorial notebooks in the docs built from this PR:

For reviewers

  • The PR title is short, concise, and will make sense 1 year later.
  • New functions are imported in corresponding __init__.py.
  • New features, API changes, and deprecations are mentioned in the unreleased
    section in CHANGELOG.rst.
  • Contributor(s) are listed correctly in __credits__ in orix/__init__.py and in
    .zenodo.json.

Signed-off-by: Håkon Wiik Ånes <hwaanes@gmail.com>
Signed-off-by: Håkon Wiik Ånes <hwaanes@gmail.com>
Signed-off-by: Håkon Wiik Ånes <hwaanes@gmail.com>
Signed-off-by: Håkon Wiik Ånes <hwaanes@gmail.com>
Signed-off-by: Håkon Wiik Ånes <hwaanes@gmail.com>
Signed-off-by: Håkon Wiik Ånes <hwaanes@gmail.com>
Signed-off-by: Håkon Wiik Ånes <hwaanes@gmail.com>
Signed-off-by: Håkon Wiik Ånes <hwaanes@gmail.com>
Signed-off-by: Håkon Wiik Ånes <hwaanes@gmail.com>
Signed-off-by: Håkon Wiik Ånes <hwaanes@gmail.com>
Signed-off-by: Håkon Wiik Ånes <hwaanes@gmail.com>
Signed-off-by: Håkon Wiik Ånes <hwaanes@gmail.com>
Signed-off-by: Håkon Wiik Ånes <hwaanes@gmail.com>
Signed-off-by: Håkon Wiik Ånes <hwaanes@gmail.com>
Signed-off-by: Håkon Wiik Ånes <hwaanes@gmail.com>
Signed-off-by: Håkon Wiik Ånes <hwaanes@gmail.com>
@hakonanes hakonanes added bug Something isn't working enhancement New feature or request theory This issue has a non-technical element documentation Relates to the documentation deprecation Deprecated functionality tests Relates to the tests documentation-examples Consider making into a documentation example labels Apr 6, 2023
@hakonanes hakonanes added this to the v0.12.0 milestone Apr 6, 2023
Signed-off-by: Håkon Wiik Ånes <hwaanes@gmail.com>
Signed-off-by: Håkon Wiik Ånes <hwaanes@gmail.com>
Signed-off-by: Håkon Wiik Ånes <hwaanes@gmail.com>
@hakonanes
Copy link
Member Author

kikuchipy's tests pass with this branch. The functionality in kikuchipy relying on reduction to Rodrigues fundamental zone is the uniform sampling of orientations. This seems to be unaffected by this change.

@hakonanes
Copy link
Member Author

hakonanes commented Apr 8, 2023

Considering the change in this PR, the notion of the first and second symmetry in OrientationRegion.from_symmetry(s1, s2=C1) changes meaning, I think. I suggest to change parameter names to start, end (Python does not allow from), and remove the default C1. C1 should really be the default for start, but then end would also need a default, which isn't possible to define.

An alternative would be to allow one or two symmetries to be passed as *args and set start to C1 if only one symmetry was passed. This would effectively maintain the syntax of OrientationRegion.from_symmetry(D6) which is currently used some places in the code.

Another alternative is two flip the order end, start=C1 (keep the current parameter order but flip where they are used internally).

Signed-off-by: Håkon Wiik Ånes <hwaanes@gmail.com>
Signed-off-by: Håkon Wiik Ånes <hwaanes@gmail.com>
Signed-off-by: Håkon Wiik Ånes <hwaanes@gmail.com>
Signed-off-by: Håkon Wiik Ånes <hwaanes@gmail.com>
@hakonanes hakonanes removed tests Relates to the tests documentation-examples Consider making into a documentation example labels Apr 8, 2023
@hakonanes
Copy link
Member Author

Talked to @anderscmathisen offline, reduction to the FZ now seems to make sense for his use.

@hakonanes hakonanes requested review from pc494 and harripj April 15, 2023 17:11
@maclariz
Copy link

Thanks @hakonanes . I hope I get some chance to test this soon on the dataset where I had the issues in the past. What is the exact version number of the new orix version which includes this change? I will probably install in a test environment initially and not destroy my old install just yet.

@hakonanes
Copy link
Member Author

That would be great. This change only lives in my fork so far, which you should be able to install into an environment like so:

pip install git+https://github.com/hakonanes/orix.git@fix-misorientation-reduce

If reviewers agree that his change is necessary, I anticipate that the change will be part of the next minor release v0.12, which should be released shortly after this PR is merged.

@maclariz
Copy link

@hakonanes I have installed in my testing environment, run it on the old data, and found that the symmetry reduction works more reliably on the Silicon misorientations dataset that was giving problems before.

@hakonanes
Copy link
Member Author

Happy to hear that, thank you for doing the tests. Please report back if you still find inconsistencies.

@hakonanes
Copy link
Member Author

FYI, @DorianDepriester checked out the changes in this PR on his calculation of weighted mean orientations in #434 (comment) and got results in line with his expectations.

@maclariz
Copy link

Sorry to spoil the party, but this new order does not work correctly in reduce for 622 symmetry.

Attached is the result of doing cluster analysis using symmetry reduction for HCP Ti using the old method.

Screenshot 2023-04-21 at 11 53 08

@maclariz
Copy link

And now here is the same workbook run with the new method. The results from the old method are quantitatively sensible. The new method is screwy. So, something about your order of rotations in symmetry reduction in hexagonal is wrong.

Screenshot 2023-04-21 at 11 54 48

@hakonanes hakonanes marked this pull request as draft April 21, 2023 11:41
@hakonanes
Copy link
Member Author

Thank you for sharing this test, @maclariz. I believe this is related to (inconsistent?) alignment of crystal axes with symmetry operations. I've encountered issues with point group -3m myself (chromium nitride in duplex steel) using the changes in this PR.

Given this issue, I suggest to wait with the change in the left/right symmetry of map_into_symmetry_reduced_zone() until we are sure of all alignments of crystal axes with symmetry operations. To easily debug these things, I suggest we implement automatic plotting of symmetry elements in the stereographic projection for all defined point groups. We can then convince ourselves of the alignments, before continuing with this change.

I'll make another PR for the changes in this PR not strictly related to the left/right symmetry change.

DorianDepriester added a commit to DorianDepriester/OptiPRISMS that referenced this pull request May 12, 2023
@hakonanes
Copy link
Member Author

To easily debug these things, I suggest we implement automatic plotting of symmetry elements in the stereographic projection for all defined point groups.

I've started to work on this in https://github.com/hakonanes/orix/tree/plot-symmetry-operations. I anticipate it will take some time.

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@pc494 pc494 mentioned this pull request Mar 26, 2024
5 tasks
@hakonanes hakonanes removed this from the v0.12.0 milestone Apr 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working deprecation Deprecated functionality documentation Relates to the documentation enhancement New feature or request theory This issue has a non-technical element
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants