Fixing C2v, adding stereographic plots for every point group, and making SYmmetry match ITOC standards

[argerlt]

Description of the change

This started as an update to the plot_symmetry_operations example, which has a few errors and also does not match the symbol conventions in the International ables of Crystallography (ITOC).

I decided to procedurally generate plots for all the point groups (most the hard work was already done, thank you @hakonanes for that), and in the process noticed orix.quaternion.symmetry.C2v was inconsistently defined compared to the other groups as well as ITOC. the common convention is to place the main cyclic rotation group around the z-axis.

In hunting that down, I also noticed some of the symmetry group names also did not match the ITOC names, and some other very minor inconsistencies in naming conventions, so I cleaned them up along with some other functions. I also added some descriptions to existing functions to help with clairity.

orix.quaternion.symmetry should be much more closely aligned with ITOC once this is pushed. Additionally, the following picture shows the auto-generatred stereographic plots for every point group

Progress of the PR

• Docstrings for all functions
• Unit tests with pytest for all lines
• Clean code style by running black via pre-commit

Minimal example of the bug fix or new feature

see orix/examples/stereographic_projection/plot_symmetry_operations.py and the plot above.

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]

Great job, the plots are very nice! It will take me some time to review, so I cannot say when I'm done.

[hakonanes]

Very good job. I have some suggestions which I think should improve maintainability and the user experience.

[hakonanes]

Now that 3.10 is the minimal Python version, we should use Vector3d | np.ndarray | list | tuple instead of Union, List, Tuple etc.

[hakonanes]

Use inner: Literal["fill", "dot", "half"] = "fill" instead.

[argerlt]

Looking back, this variable name and the options reflect the naming used for multi-colored markers in matplotlib, but don't really make sense here. Thus, I changed them to
modifier: Literal["none", "roto", "inv"] = "none") which i think is probably more clear

[hakonanes]

Why "none" and not None? And I'd use "rotation" and "inversion" as options. Not much more to type but more clear.

[argerlt]

Fixed

[hakonanes]

Use folds: Literal[1, 2, 3, 4, 6] = 2

[hakonanes]

Some suggestions to align this docstring with the orix code style:

• Start description on first line, not second like here
• Remove type hints in parameter list
• Remove blank lines between parameters in parameter list
• Start sentences with an uppercase letter

[hakonanes]

In general I prefer to make documentation "less technical" as long as no meaning is lost. Here I'd write "Vectors giving the positions of markers in a stereographic plot."

[hakonanes]

Remove extra "of"

[hakonanes]

Nice function!

But, at this point, I suggest we make a class just for easy lookup, PointGroups(subset). This can then be used to fetch any symmetry, like PointGroups().get("m-3m") -> Symmetry. What do you think? I think it is easier to work with for users, and also more maintainable than a large function.

[argerlt]

I'm having troubles seeing your vision, but I'm always down to get rid of a big ugly lookup function.

Can you lay out a super basic skeleton of the class you are suggesting? I'm guessing it's something like this but maybe you have something else in mind:

class PointGroups()
    def __init__(name: str |List(str))
    -give PG names as strings, returns list of PGs
    def get (name: str |List(str))
    - classmethod that runs get_point_group_from_space_group and returns PGs
    def subset(str)
    - classmethod that returns a subset list 

[CSSFrancis]

I like the idea, but don't know how much I like the syntax?

What if you made the _PointGroups class private and then you could

from orix import PointGroups

Which would then be an object

PointGroups.get("m3m")

The extra constructor is weird for essentially a static object right?

[hakonanes]

My suggestion was simply to change @argerlt's function into a class. The class has a constructor that takes one or more subsets (all, Laue, crystal system, etc.), provides all the relevant properties like HM, Schoenflies, Laue class, crystal system etc., can list available point groups in table (string representation), and extract a point group from the list.

E.g., if we want to plot the IPF color key for all Laue classes, we create a table of those point groups only and iterate over them via their HM symbol.

[argerlt]

What if you made the _PointGroups class private and then you could

from orix import PointGroups

Which would then be an object

PointGroups.get("m3m")

The extra constructor is weird for essentially a static object right?

@CSSFrancis ,forgot to mention it, but I liked the idea of a get function a lot and ended up doing a variation of this suggestion.

I still need to finish all of @hakonanes rewview requests, but when I'm done I can ping you and see what you think as well. In short, I think it would be nice to tie together everything related to creating, calling, or looking up crystallographic point groups into a single python class, but I want to make sure what I'm doing makes sense for everyone else's use cases as well.

[hakonanes]

With a class, we can print this string representation in the PointGroups.__repr__(). Documentation in code!

[argerlt]

I think I've tentatively resolved everything except the following two:
https://github.com/pyxem/orix/pull/563/files#r2182622890
https://github.com/pyxem/orix/pull/563/files#r2182562898

These are both going to require more more thought a feedback.

[argerlt]

@hakonanes and/or @CSSFrancis , recent realization: Symmetry.plot doesn't do what I think we think it does.

I assumed it would make a stereographic plot of symmetry elements. What it actually does is make a z-axis pole figure.

This doesn't make sense to have as a function of the Symmetry class, so I think it should be removed.

Do you think it is fine to just overwrite it with this new Symmetry.plot? Or, should I depreciate Symmetry.plot and call this new function I am writing Symmetry.plot_elements?

I'd rather do it the first way, but there is a (very unlikely) world where that might break something for someone downstream

Example:

from orix.quaternion import Orientation
from orix.quaternion.symmetry import Oh
import matplotlib.pyplot as plt
from orix.vector import Vector3d

# generate some random orientations
oris = Orientation.random(10, Oh)

# run symmetry.plot
Oh.plot(oris)

# Compare that image to a  <100> pole figure
fig, ax = plt.subplots(subplot_kw={'projection':"stereographic"})
ax.scatter(oris.symmetry.outer(oris*Vector3d.zvector()))

[argerlt]

Okay, I believe I have addressed all review comments and everything is covered and passing.

However, this has gotten rather large and hectic, and there are a lot of PR's right now, I'm going to wait until #555 #558, and #575 are merged before looking at this again.

[argerlt]

Also, the new plots look significantly nicer, in my opinion

[hakonanes]

@argerlt, are you OK with me fixing the merge conflicts and going over what you've done in a few commits pushed to your branch? I could also just fix conflicts and then make a PR to your branch, which we can then finally merge into orix in this PR?

[argerlt]

@argerlt, are you OK with me fixing the merge conflicts and going over what you've done in a few commits pushed to your branch? I could also just fix conflicts and then make a PR to your branch, which we can then finally merge into orix in this PR?

I think it might be better if I break this monster out into a 2 smaller and cleaner new PRs, just to avoid polluting the commit history. After that though, feel free to push whatever commits you want to fix things, I think we are more or less in agreement on all the big points.

I was hoping to start this today, after I finished reviewing #588. That said, if tomorrow rolls around and I haven't done anything, feel free to just fix this PR instead.

[hakonanes]

No problem leaving it for a few days! I'm not sure you'd pollute our git history with this PR... Given the limited resources we have for this project, I'm fine not thinking too much about that.

[argerlt]

I was hoping to start this today,

Well, that was a lie on my part.

This has been sitting around for too long, I'm just going to fix it here. "refactor and split" was a nice dream, but a year in, and I'm ready to move on.

Few outstanding issues:

• Given some time to look back, I think PointGroup.__init__, PointGroup.get, and PointGroup.get_set are a bit clunky as-is

• PointGroups('groups')[0].name should give the name(s), but doesn't. there are some other symmetry values like this that probably need fixing.

• Language, formatting, etc.

It is far enough along though for a re-review.

[argerlt]

pre-commit.ci autofix

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB