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

Fix inherited methods in the Orientation class #373

Open
hakonanes opened this issue Aug 4, 2022 · 8 comments
Open

Fix inherited methods in the Orientation class #373

hakonanes opened this issue Aug 4, 2022 · 8 comments
Labels
bug Something isn't working enhancement New feature or request

Comments

@hakonanes
Copy link
Member

hakonanes commented Aug 4, 2022

The Orientation class inherits methods and properties from Rotation, Quaternion and Object3d. Currently, these methods and attributes aren't handled properly (e.g. symmetry isn't kept):

Rotation

  • __mul__(): which object with which symmetry (if any) should be returned?

Quaternion

  • mean()

Object3d

Please add methods and attributes missing from the lists.

@argerlt
Copy link
Contributor

argerlt commented Aug 8, 2022

question about this because it came up, what should happen in the following situation?

from orix.quaternion import Orientation, symmetry

pg432 = symmetry.O
pg622 = symmetry.get_point_group(180)

o1 = Orientation((1, 0, 0, 0), symmetry=pg432)
o2 = Orientation((1, 0, 0, 0), symmetry=pg622)
r1 = Rotation((1, 0, 0, 0))
r2 = Rotation((1, 0, 0, 0))
m12 = o1 - o2
m21 = o2 - o1

o1 - o2 gives a misorientation with symmetry=(432,662). Makes sense
o1 * o1 should give a symmetry of 432, but gives 1. this should be fixed
o1 * r1 gives a rotation without symmetry. Makes sense, we are reorienting a rotation, and it agrees with how MTEX works.
m12 * r1 also gives a rotation
r1 * m12 has the same problem and solution as r1 * o2, just use the symmetry of the object on the right.

what should o1 * o2 produce? should it have the symmetry of o1, o2, or a supergroup of both symmetries together? should it throw an error MTEX-style, or just expect users to be aware of the dangers?

Also, same question for m12 * o1, o1 * m12.

Also not sure how m12 * m21 should work. this example isn't the best, but I'm just thinking of the case wehre there are 4 different symmetry objects.

If you let me know how you want it, I can write this code, it was on my todo list anyway. functions to fix are mean and __mul__. are there any others?

@hakonanes
Copy link
Member Author

hakonanes commented Aug 8, 2022

Thank you for listing these use cases, we should provide sensible returns for all of them. I've updated the top comment with your suggestions.

This #249 (comment) by @harripj on the (undocumented) misorientation convention in orix might be relevant. We discussed somewhere, and decided I believe, to retire the o2 - o1 syntax and make o2 * ~o1 the only supported operation to get the misorientation bringing o1 into o2.

@hakonanes hakonanes added bug Something isn't working enhancement New feature or request labels Aug 8, 2022
@argerlt
Copy link
Contributor

argerlt commented Aug 9, 2022

Okay, my two cents:

I really like the orix convention that for all combinations of Rot/Ori/Mis, AB gives the same class as B. in that sense, I don't care too much for the o1~o2, since it implies ~o1*o2 should do the inverse, and it also goes against the class inheritance convention just described. In that way, I like having o1 - o2, because it describes a different process, and refers to the delta between the two orientations, which is more or less what a misorientation is.

That said, I brought it up to @paytonej, (my old boss), and they disagree, they see AB as matrix multiplication, and in that sense (o1o2.T) should just give a misorientaion. this more or less agrees with what @harripj is saying, and also makes sense too.

@hakonanes
Copy link
Member Author

I like the interpretation of o2 * ~o1 as a matrix multiplication because an orientation (in orix) is a transformation from the sample to the crystal: first we invert o1 to get the transformation from that crystal to the sample, and then we go from the sample to the other crystal represented by o2. However, previously, I often forgot the order and which to invert. For less frequent users of orix, having the o2 - o1 syntax might be simpler to remember.

@harripj
Copy link
Collaborator

harripj commented Aug 9, 2022

AB being matrix multiplication makes the most sense from a mathematical perspective, but not from a NumPy perspective, and I think users would expect agreement with NumPy when using orix.

I like your explanation @hakonanes and maybe it could find its way into the docs at some point (if not there already!). I would also be in favour of keeping the subtract syntax as it is more simple for the user. I also think the syntax implies a directionality, o2 - o1 should take me to o2 from o1, whereas I think this is somewhat lost in the multiplication congruent.

@argerlt
Copy link
Contributor

argerlt commented Aug 9, 2022

Alright, I like that logic.

Followup to all this then, for symmetry inheritance, I was going to write everything to stick to the following rules:

  1. when doing A*B, the resulting AB is of the same class as B.
  2. if AB is a rotation, ignore steps 3 and 4
  3. supergroup the symmetries from A and B to define the new object's symmetry as follows:
L_CS = (A._symmetry[0], B._symmetry[0])  
R_CS = (A._symmetry[1], B._symmetry[1])
AB._symmetry = (L_CS, R_CS)
if B.__class__ == Orientation, AB._symmetry[0] = C1
  1. if option 2 causes the symmetry to change from the symmetry of B, throw a warning that users can silence, so they know they are probably doing something bad, but doesn't actually stop them.

This way, you CAN multiply orientations from different crystal symmetries (it drives me nuts that MTEX wont allow this), but it warns you that you are making a dangerous move. Also, it makes it obvious when a misorientation or orientation was multiplied by an improper symmetry object, but doesn't punish you for multiplying by something with a symmetry of C1.

@hakonanes
Copy link
Member Author

I think your rules look reasonable, apart from rule 3 which I don't understand: Will a resulting misorientation's _symmetry attribute be a tuple of 2-tuples of two symmetries, as in ((Symmetry, Symmetry), (Symmetry, Symmetry))?

@argerlt
Copy link
Contributor

argerlt commented Aug 20, 2022

There has to be a more elegant way to do this, but I imagined it like this:

o1._symmetry = S1
o2._symmetry = S2
m12._symmetry[0] = Symmetry(np.vstack([S1[0].data, S2[0].data]))
m12._symmetry[1] = Symmetry(np.vstack([S1[1].data, S2[1].data]))

would work the same for o1 * o1 as well, the _symmetry[0] would just be C1*C1, and the second two would form a supergroup of the resulting o12.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants