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

Feature: Gridding for rotation lists #47

Merged
merged 136 commits into from
Jan 22, 2020
Merged

Conversation

pc494
Copy link
Member

@pc494 pc494 commented Jan 13, 2020


name: Feature: Gridding for rotation lists
about: This PR adds easier user function for providing rotation_list


WIP

Release Notes

major
new feature
Summary: Introduce 3 gridding functions, see the PR for details.

What does this PR do? Please describe and/or link to an open issue.

  • Deletes all streographic triangle related functionality

  • Adds 3 new gridding functions in the file rotation_list_generators.py these are:
    1) full fundamental zone
    2) a small region around a known orientation
    3) around the beam for a known orientation

  • To support this, vectorisation of several transform3d functions is provided, as well as two smallish classes (Euler and Axangle) in rotation_conversion_utils.py

Describe alternatives you've considered

  • Internal work is done using vectorization of the code in transforms3d, see rotation_conversion_utils.py, these are tested against the appropriate for loops. I could have either (1) used orix.

  • "Gridding" could be done in any number of spaces, with various levels of correctness. I've chosen to use euler angles for everything fundamental zone code (anticipating speed ups that rely on this) and axangles for gridding code (for speed, as generating subsets of the space in euler angles is a challenge)

@pc494 pc494 added the enhancement New feature, request, or improvement label Jan 13, 2020
@dnjohnstone
Copy link
Member

@EirikOpheim @tinabe - for reference.

@dnjohnstone dnjohnstone added this to the v0.2.0 milestone Jan 13, 2020
@pc494 pc494 requested a review from dnjohnstone January 20, 2020 20:22
Copy link
Member

@dnjohnstone dnjohnstone left a comment

Choose a reason for hiding this comment

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

A few comments straight off the bat, will keep looking further.

.coveragerc Show resolved Hide resolved
diffsims/generators/rotation_list_generators.py Outdated Show resolved Hide resolved
Between 1 and 230

resolution : float
The 'resolution' of the grid (degrees)
Copy link
Member

Choose a reason for hiding this comment

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

Can we specify what this means more precisely? Is it the misorientation between points in the grid?

diffsims/generators/rotation_list_generators.py Outdated Show resolved Hide resolved

Returns
-------
structure_library : StructureLibrary
Structure library for the given phase names, structures and orientations.
"""
return StructureLibrary(self.phase_names, self.structures, orientations)

def get_orientations_from_stereographic_triangle(self, inplane_rotations, resolution):
Copy link
Member

Choose a reason for hiding this comment

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

Why is this function getting fully removed? My impression is that this approach is the one used in the Meng & Zuo paper about improving automated indexation and in Rauch papers - so I doubt it's wrong in principle and in the short term, I'd have a preference for supporting it as a "community standard", but only if it's not a massive time sink.

Copy link
Member Author

Choose a reason for hiding this comment

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

I missed this at first pass. I'm happy to keep it, will undo the deletion and make corrections in a different PR

diffsims/utils/fundemental_zone_utils.py Outdated Show resolved Hide resolved
diffsims/utils/fundemental_zone_utils.py Outdated Show resolved Hide resolved
diffsims/utils/gridding_utils.py Show resolved Hide resolved
@pc494
Copy link
Member Author

pc494 commented Jan 22, 2020

This PR is now very big, @dnjohnstone would you be happy to merge it if:

a) I undeleted the get_streographic bits of code, with intention to improve in a new PR
b) marked the get_grid_around_beam_direction() as NotImplemented and fixed in a new PR

@dnjohnstone
Copy link
Member

This PR is now very big, @dnjohnstone would you be happy to merge it if:

a) I undeleted the get_streographic bits of code, with intention to improve in a new PR
b) marked the get_grid_around_beam_direction() as NotImplemented and fixed in a new PR

a) I'm happy for you to remove the get_stereographic temporarily and raise an issue/new PR to replace it because I think, from our other discussion, that what was there is actually wrong in current implementation - if it's not actually wrong then undeleted is good.

b) yes you can do that, or even just raise a warning saying it only works for the cubic case? And raise an issue/ new PR to fix it.

Then yes, happy to merge

@pc494
Copy link
Member Author

pc494 commented Jan 22, 2020

Issues #51 and #52 discuss the points mentioned here & will be tackled for 0.2.0. This PR is now ready to be reviewed in full and merged from my perspective.

@dnjohnstone dnjohnstone merged commit 746efbc into pyxem:master Jan 22, 2020
@pc494 pc494 deleted the feature-gridding branch January 25, 2020 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature, request, or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants