orix icon indicating copy to clipboard operation
orix copied to clipboard

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

Open hakonanes opened this issue 2 years ago • 13 comments

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:

  • https://github.com/pyxem/orix/issues/368#issuecomment-1163212482
  • https://github.com/pyxem/orix/issues/434#issuecomment-1492180192
  • #437

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

  • [x] Docstrings for all functions
  • [x] Unit tests with pytest for all lines
  • [x] Clean code style by running black via pre-commit
  • [x] Scrutiny of the change by others, like testing on their own problems. I've reached out to @maclariz, @DorianDepriester and @anderscmathisen in the above mentioned issues for their feedback.

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.

hakonanes avatar Apr 06 '23 20:04 hakonanes

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 avatar Apr 07 '23 11:04 hakonanes

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).

hakonanes avatar Apr 08 '23 09:04 hakonanes

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

hakonanes avatar Apr 15 '23 17:04 hakonanes

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.

maclariz avatar Apr 17 '23 11:04 maclariz

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.

hakonanes avatar Apr 17 '23 11:04 hakonanes

@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.

maclariz avatar Apr 17 '23 15:04 maclariz

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

hakonanes avatar Apr 17 '23 16:04 hakonanes

FYI, @DorianDepriester checked out the changes in this PR on his calculation of weighted mean orientations in https://github.com/pyxem/orix/issues/434#issuecomment-1513203390 and got results in line with his expectations.

hakonanes avatar Apr 19 '23 10:04 hakonanes

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 avatar Apr 21 '23 10:04 maclariz

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

maclariz avatar Apr 21 '23 10:04 maclariz

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.

hakonanes avatar Apr 21 '23 11:04 hakonanes

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.

hakonanes avatar Jun 01 '23 18:06 hakonanes

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB