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

Add Symmetry plot method #306

Merged
merged 28 commits into from Mar 29, 2022
Merged

Conversation

harripj
Copy link
Collaborator

@harripj harripj commented Mar 22, 2022

Description of the change

As discussed in #303 and #171, it would be a good idea to add a plot method to Symmetry to display symmetry elements in the crystal reference frame under the stereographic projection.

Closes #171.

Progress of the PR

Minimal example of the bug fix or new feature

>>> from orix.quaternion import symmetry
>>> symmetry.O.plot()

image

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.

harripj added a commit to harripj/orix that referenced this pull request Mar 22, 2022
@harripj harripj added enhancement New feature or request documentation Relates to the documentation labels Mar 22, 2022
@harripj harripj added this to the v0.9.0 milestone Mar 22, 2022
@harripj harripj requested a review from hakonanes March 22, 2022 13:34
Copy link
Member

@hakonanes hakonanes left a comment

Choose a reason for hiding this comment

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

Very helpful and nice visualization of symmetry operations! All requests and comments are related to documentation and tests.

Should add plot to the list of Symmetry methods below from_generators in the reference.

orix/quaternion/symmetry.py Outdated Show resolved Hide resolved
orix/quaternion/symmetry.py Outdated Show resolved Hide resolved
orix/quaternion/symmetry.py Outdated Show resolved Hide resolved
orix/quaternion/symmetry.py Outdated Show resolved Hide resolved
orix/quaternion/symmetry.py Outdated Show resolved Hide resolved
orix/tests/quaternion/test_symmetry.py Outdated Show resolved Hide resolved
doc/point_groups.ipynb Outdated Show resolved Hide resolved
doc/point_groups.ipynb Outdated Show resolved Hide resolved
doc/point_groups.ipynb Outdated Show resolved Hide resolved
doc/point_groups.ipynb Outdated Show resolved Hide resolved
@hakonanes
Copy link
Member

hakonanes commented Mar 25, 2022

Seems like GitHub has submitted all my comments twice. I suggest to leave the extra comments, in case both are deleted when one is deleted...

EDIT: Seems like its only on my end, since the extra comments are labeled "Pending".

EDIT2: This is just the case for some of them it seems.

@harripj
Copy link
Collaborator Author

harripj commented Mar 25, 2022

Doc build error looks like it is part of jinja2 or nbconvert and is being experienced elsewhere recently: sphinx-doc/sphinx#10289

@harripj
Copy link
Collaborator Author

harripj commented Mar 25, 2022

Doc build error looks like it is part of jinja2 or nbconvert and is being experienced elsewhere recently: sphinx-doc/sphinx#10289

Looks like a fairly minor fix. I suggest we wait until nbconvert pushes the patch which will hopefully sort everything from our side without change, An alternative is to pin jinja2 to 3.0 in requirements, but it seems that like this will be cleared up pretty quickly.

@hakonanes
Copy link
Member

I suggest we wait until nbconvert pushes the patch which will hopefully sort everything from our side without change

I agree. Checked the notebook locally, just had one important comment.

@hakonanes
Copy link
Member

After the notebook change, I have a final suggestion: could you update the locations of other relevant packages references in conf.py

from

    "h5py": ("http://docs.h5py.org/en/stable/", None),
    "matplotlib": ("https://matplotlib.org", None),
    "numpy": ("https://docs.scipy.org/doc/numpy", None),
    "scipy": ("https://docs.scipy.org/doc/scipy/reference", None),

to

    "h5py": ("https://docs.h5py.org/en/stable", None),
    "matplotlib": ("https://matplotlib.org/stable", None),
    "numpy": ("https://numpy.org/doc/stable", None),
    "scipy": ("https://docs.scipy.org/doc/scipy", None),

Sphinx tells us nicely that they have changed when running make html.

@harripj
Copy link
Collaborator Author

harripj commented Mar 25, 2022

Doc build error looks like it is part of jinja2 or nbconvert and is being experienced elsewhere recently: sphinx-doc/sphinx#10289

Looks like a fairly minor fix. I suggest we wait until nbconvert pushes the patch which will hopefully sort everything from our side without change, An alternative is to pin jinja2 to 3.0 in requirements, but it seems that like this will be cleared up pretty quickly.

NB this is the PR in question jupyter/nbconvert#1737.

@harripj
Copy link
Collaborator Author

harripj commented Mar 28, 2022

NB this is the PR in question jupyter/nbconvert#1737.

This appears to have been fixed.

@harripj harripj mentioned this pull request Mar 28, 2022
15 tasks
@hakonanes
Copy link
Member

This PR can now use reprojection functionality of vectors on the other hemisphere!

@hakonanes
Copy link
Member

Just ping me whenever this is ready for review.

@harripj
Copy link
Collaborator Author

harripj commented Mar 29, 2022

@hakonanes I think this is good for review. I will look at #308 now too.

@harripj harripj requested a review from hakonanes March 29, 2022 13:54
Copy link
Member

@hakonanes hakonanes left a comment

Choose a reason for hiding this comment

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

This is a very good way to visualize symmetry operations; I'm sure I'll use it many times!

I've suggested an insignificant change to the user guide notebook. Performing the change isn't required to merge, but I think it shows the user a slightly simpler way to reflect a vector about the projection plane.

doc/point_groups.ipynb Outdated Show resolved Hide resolved
@hakonanes hakonanes requested a review from pc494 March 29, 2022 14:53
Copy link
Member

@pc494 pc494 left a comment

Choose a reason for hiding this comment

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

This looks good, assuming the tests green light. I'll let @hakonanes merge.

@hakonanes hakonanes merged commit 40e4807 into pyxem:master Mar 29, 2022
@harripj harripj deleted the add_Symmetry_scatter branch March 29, 2022 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Relates to the documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Plotting of symmetry elements
3 participants