Fixing C2v, adding stereographic plots for every point group, and making SYmmetry match ITOC standards #563
Conversation
see chapter 12, as wel as figure 10.3.2 of International Tables of Crystallography, Volume A, for details on this discrepency, but The Cyclic rotation axis should be the z axis, not the x axis, in the standard form.
|
Great job, the plots are very nice! It will take me some time to review, so I cannot say when I'm done. |
|
|
||
|
|
||
| # Iterate through the 32 Point groups | ||
| for i, pg in enumerate(point_groups): |
There was a problem hiding this comment.
Great that you did this without hard-coding!
I suggest to replace the default symmetry plot with this one. The current symmetry plot can be available by setting a parameter, like mode: Literal["operations", "points"] = "operations".
The proper place for this algorithm would be in the stereographic plot file, I think.
There was a problem hiding this comment.
Okay, I thought about this for a long time. You are right this should, somehow, be functionalized. However, I don't want a single hardcoded "add symmetry markers" function because:
1) This example follows ITOC, which is marginally different from "Structure of Materials" (see mmm for example) and also neither of those methods hold up perfectly when applied to off-axis or non-standard plot orientations (do you plot all the inversion markers? or only some? what if all the rotation axes are all in-plane with xy?).
2) I made some choices with regard to coloring that others might not like. It would be nice to allow users to control those.
3) I think my method might actually break a bit for systems where the z-axis is not pointing out of the page, specifically in how the rotation markers are orientated.
I think I'm going to try breaking this into two parts:
Symmetry.plottable_symmetry_elements() to run the algorithm to find the elements to plot
StereographicPlot.all_symmetry_markers(sym) which will run the output of the
8000
above function in a for loop and have some additional choices for the user to change, but also explicitly show how to run the equivalent with more control using the existing StereographicPlot.symmetry_markers(sym)
Edit: ignore all this. as mentioned below, this was based on a misinterpretation, and no longer applies.
There was a problem hiding this comment.
I still think we get the most out of your hard work by getting these right if we make them available via Symmetry.plot().
- Do you have an example in which off-axis or non-standard plot orientations are used? (I think it is much better that we make a choice and present that to the user than us not making any choice and the user getting nothing.)
- We can add coloring options for the various symmetry operation types as parameters (rotation, mirror, inversion).
- Currently we only support the c-axis out of plane, so these plots are all in line with the current supported crystal axes alignments.
There was a problem hiding this comment.
AH! I misunderstood you the first time! I thought you were suggesting adding an option for automatic markers to StereographicPlot.scatter, StereographicPlot.plot, etc, which I wasn't crazy about. What you actually suggested was replacing the function Symmetry.plot. That's a great plan, I did not even realize that was a function! Sorry, should have read more carefully.
Okay, yup, I'm 100% on board now. I'll fix that up ASAP
Do you have an example in which off-axis or non-standard plot orientations are used?
Not anymore, I was vaguely worried if someone wanted a custom StereographicPlot at an off-axis view, it could potentially mess up how the markers get rotated, as well as rules for where you do and don't add inversion dots to symmetry markers, but for the finite set of possible crystallographic Symmetry plots, i think it's fine.
We can add coloring options for the various symmetry operation types as parameters (rotation, mirror, inversion).
Agreed
Currently we only support the c-axis out of plane, so these plots are all in line with the current supported crystal axes alignments.
Also agreed
There was a problem hiding this comment.
The proper place for this algorithm would be in the stereographic plot file, I think.
So, a function plot.stereographic_plot.get_markers_from_symm(), that then gets caled by the class function
Symmetry.plot ? I like that, I can give it a go.
There was a problem hiding this comment.
Yes, something like that sounds good. I'd even make the stereographic plot method private! We can instead give more flexibility to users when they ask for it.
There was a problem hiding this comment.
Done. I ended up changing plot.stereographic_plot.get_markers_from_symm(), to Symmetry.get_symmetry_elements() because the output was useful for other functions that use Symmetry as an input.
orix/plot/stereographic_plot.py
Outdated
|
|
||
| def __init__( | ||
| self, | ||
| v: Union[Vector3d, np.ndarray, list, tuple], |
There was a problem hiding this comment.
Now that 3.10 is the minimal Python version, we should use Vector3d | np.ndarray | list | tuple instead of Union, List, Tuple etc.
orix/plot/stereographic_plot.py
Outdated
| v: Union[Vector3d, np.ndarray, list, tuple], | ||
| size: int = 1, | ||
| folds=2, | ||
| inner="fill", |
There was a problem hiding this comment.
Use inner: Literal["fill", "dot", "half"] = "fill" instead.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Why "none" and not None? And I'd use "rotation" and "inversion" as options. Not much more to type but more clear.
There was a problem hiding this comment.
how about modifier: Literal[None,"none","rotation","rotoinversion","inversion""] = "none") then? first three map to None, second two map to rotoinversion and inversion.
orix/plot/stereographic_plot.py
Outdated
| self, | ||
| v: Union[Vector3d, np.ndarray, list, tuple], | ||
| size: int = 1, | ||
| folds=2, |
There was a problem hiding this comment.
Use folds: Literal[1, 2, 3, 4, 6] = 2
orix/plot/stereographic_plot.py
Outdated
|
|
||
| class SymmetryMarker: | ||
| """ | ||
| Symmetry element markers to plot in stereographic representations of |
There was a problem hiding this comment.
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
orix/plot/stereographic_plot.py
Outdated
| Parameters | ||
| ---------- | ||
| v (Vector3d object) | ||
| Vector3d object used for placing marker in StereographicPlot |
There was a problem hiding this comment.
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."
| @@ -136,10 +138,12 @@ def euler_fundamental_region(self) -> tuple: | |||
| "211": (360, 90, 360), # Monoclinic | |||
| "121": (360, 90, 360), | |||
| "112": (360, 180, 180), | |||
| "2": (360, 180, 180), | |||
There was a problem hiding this comment.
Why is this needed? Isn't the proper point group always one of 211, 121, or 112?
There was a problem hiding this comment.
Yes, but a bunch of the checks in Symmetry don't actually check for symmetry operators, they just check that Symmetry.name matches something in their lookup tables. The ITOC name for C2 is 2 so I wanted to include it, but functions throughout orix, and also likely in people's own code, would break if i removed '112' as an option, so I just added both to the lookup table for redundancy.
I think the right thing to eventually do (though not as part of this PR) would be to remove all name-based lookup tables form functions in Symmetry, but that is going to be a major project.
orix/quaternion/symmetry.py
Outdated
| # NOTE: ORIX uses Schoenflies symbols to define point groups. This is partly | ||
| # because the notation is short and always starts with a letter (ie, they | ||
| # make convenient python variables), and partly because it helps limit | ||
| # accidental misinterpretation of of Herman-Mauguin symbols as space group |
orix/quaternion/symmetry.py
Outdated
| # fmt: on | ||
| _proper_groups = [C1, C2, C2x, C2y, C2z, D2, C4, D4, C3, D3x, D3y, D3, C6, D6, T, O] | ||
|
|
||
| def get_point_groups(subset: str = "unique"): |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
orix/quaternion/symmetry.py
Outdated
| in "all", of which the other lists are subsets of. | ||
|
|
||
|
|
||
| +-------------+--------------+---------------+------------+------------+ |
There was a problem hiding this comment.
With a class, we can print this string representation in the PointGroups.__repr__(). Documentation in code!
|
I think I've tentatively resolved everything except the following two: These are both going to require more more thought a feedback. |
|
@hakonanes and/or @CSSFrancis , recent realization: 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 Do you think it is fine to just overwrite it with this new I'd rather do it the first way, but there is a (very unlikely) world where that might break something for someone downstream Example: |
|
The plot is meant to show equivalent directions in a crystal, given one direction. Just like the points in the plots from McHenry and De Graef: #456 (comment). I think the option to add:
would be best. |
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.C2vwas 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
Minimal example of the bug fix or new feature
see
orix/examples/stereographic_projection/plot_symmetry_operations.pyand the plot above.For reviewers
__init__.py.section in
CHANGELOG.rst.__credits__inorix/__init__.pyand in.zenodo.json.