Making Orientations a child class of Rotations, not Misorientations

[argerlt]

I'd like to write some class method constructors for Misorientations which generates them from paired lists or arrays of orientations, as well as some other misorientation-specific functions.

Right now, Orientations use Misorientations as a base class, which violates Liskov substitution principle and leads to odd edge cases such as calling Orientation.equivalent(ori) and getting a doc string referencing misorientations. It would really make more sense to make both use rotations as a base class, as opposed to overwriting their shared functions. For my specific case, this would make writing the classmethods a lot more straight forward.

I was about to reorganize this for my own use anyway, but before i did, I wanted to check if there was a bigger reason I didn't understand/appreciate for misorientations being a base class for orientations.

Thanks, I'm a big fan of orix.

[hakonanes]

Hi @argerlt,

I've seen your hackathon fork, and think it's great that you're reaching out to us now, even during the hackathon!

As far as I know, the choice of Orientation subclassing Misorientation was simply because, at the time orix was initially written, it was sufficient to consider orientations as misorientations where the initial "crystal" was the sample reference frame:

https://github.com/pyxem/orix/blob/14639a15a4c66c87891debb8656ef187c3b6e9ea/orix/quaternion/orientation.py#L473-L474

With the initial functionality, this made sense and made for a slightly simpler implementation. The functionality and use cases of mainly Orientation have grown quite a lot since then... I think you raising this issue is a sign that we should do the change you suggested. I have no arguments against it.

@pc494, who've been part of the project from the beginning might have some additional corrections to my "historical review".

[argerlt]

Thanks, I'll give it a try.

And yeah, as a general FYI, we are trying to recreate a very specific piece of software that exists in MTEX but not in python, and Orix was agreed upon as the best starting point for a pythonic equivalent. As such, most of those forks and subforks contains a LOT of things you won't find useful, but the ideal goal is after this finishes up, myself and Steve Niezgoda will take the parts that might be useful to the greater Orix crowd and start adding those as pull requests.

[harripj]

And yeah, as a general FYI, we are trying to recreate a very specific piece of software that exists in MTEX but not in python, and Orix was agreed upon as the best starting point for a pythonic equivalent. As such, most of those forks and subforks contains a LOT of things you won't find useful, but the ideal goal is after this finishes up, myself and Steve Niezgoda will take the parts that might be useful to the greater Orix crowd and start adding those as pull requests.

This sounds excellent! Looking forward to it!

[hakonanes]

Thanks, I'll give it a try.

Thank you. I'd wait a couple days though, until @pc494 has gotten the possibility to give his opinion. If he agrees that this is the best way forward, feel free to open a PR early so that we can give pointers early on!

[pc494]

Sorry to jump in late with an opinion against the flow, but the choice of inheritance of Orientation from Misorientation was fairly deliberate and I'm not convinced that it should be changed with some serious reflection.

The idea was that Rotation describes a process that occurs on a object where as (Mis)orientation describes a difference between two objects (two physical object for Misorientation, a lab frame and an object for an Orientation). This means that documentation referring to misorientation applies equally well to orientation, understood as they are as misorientations from a lab frame.

While I'm committed to that as a way of understanding the situation, I've yet to work over any programming problems it causes and so it may make sense to change things internally. @argerlt would you be able to scope out the programming problems in a little more detail?

[hakonanes]

I saw your issue https://github.com/mesoOSU/Mart2Aust_Hackathon/issues/48, @argerlt. Don't know whether you've moved on beyond what's in that issue, but a misorientation m12 rotating a crystal of orientation o1 into a crystal of orientation o2 can be achieved like so

>>> import numpy as np
>>> from orix.quaternion import Misorientation, Orientation, symmetry

>>> o1 = Orientation.from_axes_angles((0, 0, -1), np.pi / 2, symmetry.Oh)
>>> o1
Orientation (1,) m-3m
[[ 0.7071  0.      0.     -0.7071]]
>>> np.rad2deg(o1.to_euler())
array([[90.,  0.,  0.]])

>>> o2 = Orientation.from_axes_angles((0, 0, -1), np.pi, symmetry.Oh)
>>> o2
Orientation (1,) m-3m
[[ 0.  0.  0. -1.]]
>>> np.rad2deg(o2.to_euler())
array([[180.,   0.,   0.]])

>>> o12 = o2 * ~o1  # ~ inverts the orientation, equivalent to inv() in MTEX
>>> m12 = Misorientation(o12, (o1.symmetry, o2.symmetry))
>>> m12
Misorientation (1,) m-3m, m-3m
[[ 0.7071  0.      0.     -0.7071]]
>>> np.rad2deg(m12.to_euler())
array([[90.,  0.,  0.]])

I agree that this approach is not well explained in the documentation. To tackle this we should add a "Misorientation" topic similar to the "Orientation" topic, and start to add at least some basic handling of misorientations there.

When it comes to the operation o2 * ~o1, I think we should try to make it so that the operation returns a Misorientation instance, if possible. I believe this requires two things:

1. Keeping track of inverted orientations via a setter so that whether an orientation is inverted or not can be set regardless of whether ~ has been used, so we don't loose any existing flexibility
2. When either of the two orientations being multiplied are inverted, we return a misorientation instance with the two orientations' symmetries set as the misorientation symmetries

[argerlt]

Sorry for the radio silence. First off, @hakonanes: Thanks for that code snippet. I eventually found I could do "mis = R.mul(o1, o2.inv())" and we all started using variations of that, but this is significantly cleaner. Also, now that I know you can do that, it seems really intuitive, but finding it was rough. As I mention in point 2 below, making sure no one else ever got lost finding this functionality was why I originally started this Issues topic.

@pc494, my logic for switching the class inheritance is three-fold. First is philosophical, second is practical, third is very specific to my needs:

1) As I mentioned, the current class inheritance violates Liskov substitution principle. I know this sounds pedantic, but its a part of SOLID principles for a reason. The fact that Orientation has to overwrite functions of Misorientation to work correctly opens up Orientations to future breaks. Image if someone changed Orientation.scatter to Orientation.scatterplot, and suddenly the command Orientation.scatter used Misorientation.scatter, which would (I believe) throw an error. Same goes for get_distance_matrix, distance, and symmetry, they make the code less adaptable.

2) practical reason: I really like how I can use orix.symmetry to generate Symmetry objects. Replicating that so things like "misorientation.from_orientations" autocreated objects of the Misorientation class would be a clever way to show users ways they can generate Misorientation objects which they can quickly find in the documentation. The documentation for the function could then say "Equivalent to calling 01*~02", similar to how numpy added the array.T method to the documentation for numpy.Transpose..

3) For my own work, I have to write ODFs and MODFs in python, ideally using Orix. This is way easier to do when they are separate classes that don't inherit each other.

Also, in reference to your point:

The idea was that Rotation describes a process that occurs on a object where as (Mis)orientation describes a difference between two objects (two physical object for Misorientation, a lab frame and an object for an Orientation). This means that documentation referring to misorientation applies equally well to orientation, understood as they are as misorientations from a lab frame.

I agree making redundant docstrings should be avoided, 100% with you on that point. However, the facts that "symmetry" has to be overwritten between the functions, that they have different possible symmetries, and that m(o1,o2) has meaning but o(m1,m2) does not suggests they are not equivalent and should not have equivalent documentation.

I think its fair to be against the "misorientation.from_orientations" suggestion, but "Misorientation.from_orientations" would for sure be a useful function, and that would definitely violate LSP with the current structure. Trying to add that function while avoiding an infinite recursion issue is what originally made me want to suggest change in inheritance.

One way to fix all this without writing a lot of redundant code or function inheritance would be if both Orientation and Misorientation inherited from some middle class that contained shared functions which don't belong in Rotation, but not sure what you would call that. BaseOrientation maybe? seems like there should be a better name, but I'm coming up blank.

[pc494]

Okay, I think I've been won around here. For the original purpose of orix, it certainly made sense to highlight the similarity between these two objects. But now, especially if the package moves in the MDF, ODF direction this is no longer the case. I think/agree there is some merit to a class (potentially an abstract and/or private one) that sits above Orientation/Misorientation to provide some functionality, but I'll leave that as a suggestion for those who implement the change.

[argerlt]

Hey, sorry for the long radio silence, but quick update/question for the experts:

After spending a few weeks poking around on this, it seemed to me the best solution was to move all the symmetry aware functions out of Misorientation and into Rotation. This is sort of in line with the discussion in #254, Rotation would still have things like "map_into_symmetry_reduced_zone", rotations would just map to themselves because symmetry= None or C1

However, Symmetry is a subclass of Rotation, so doing what was described above would cause circular imports.

@hakonanes and @pc494, Is there some reason it wouldn't make sense to make symmetry instead inherit from Quaternion, then just moves the to/from and angle functions that Symmetry uses down into Quaternion as well? this way, Rotation could contain ALL the symmetry-aware functions, which Ori and Misori would inherit, and we could reduce the number of overwritten and repeated code.

Other option is to create a function called _Ation between Quaternion and Rotation in the inheritance line, but that seems like it further convolute things. Still, if there is a good reason not to touch Quaternion, would be a good second option.

Thoughts?

[hakonanes]

For Symmetry to inherit from Quaternion we need to identify the Rotation methods the former uses currently, like unique(). After those are handled, if possible, I don't see any reason not to make that change.

There are a lot of caveats and gotchas in orix, and I'm happy for any contributions trying to remove some of those.

[pc494]

Symmetry inheriting from Quaternion rather than Rotation is fine by me.

[hakonanes]

Continuing discussion from https://github.com/pyxem/orix/issues/373#issuecomment-1209674268 and https://github.com/pyxem/orix/issues/373#issuecomment-1221217371 (keeping that issue focused on Orientation inherited methods):

Is it enough to add a special case in Orientation.__mul__() (currently inherited from Rotation, code) and assign the symmetries like in Orientation.__sub__() (code)?

We should warn the user when they don't multiply an orientation with an inverted one, and for this we need an Orientation._inverted attribute (private, should only change when Orientation.__inv__() is called), mentioned in https://github.com/pyxem/orix/issues/345#issuecomment-1139718365.

[argerlt]

Coming back to this after a long break. Quick recap: 1) Long vertical Class dependencies relying on function overwrites can make code break easily. 2) Cannot to the MTEX-style o1*o2=m12 or similar "magic" dunder functions with orientations without circular import error 4) ODFs and MODFs are easier to add when Orientation and Misorientation are separate classes.

Current Setup

This is the current class inheritance layout:

Proposed Alternate

At one point, we agreed it made sense to make the Symmetry class a child of Quaternions, not Rotations, which would allow for a layout like this:

I like this a lot, it avoids long vertical overwrites of functions like "flatten", and allows users to write Rot/Mis/Ori specific functions. I'm over half done with a pull request that would implement this. However, it does mean giving Quaternion the 'improper' property, which is not an uncommon thing in other implementations, but I think should be a conversation before I moved the improper property from Rotation to Quaternion. @pc494 @hakonanes

I have some other alternatives I thought of if an improper 'Quaternion` is a deal breaker, but wanted feedback on this first.

[hakonanes]

Hi @argerlt, its great that you have found time to look at class inheritance in orix!

If you can, I suggest you open a PR as soon as you have something you want discussed. As @pc494 always says, this ensures we know what others are doing so that we pull in the same direction.

I have a conference next week, but will get back to this discussion after that.

without circular import error

We can avoid this by importing within the method instead of at the top of a file. The reason why this works, I think, is that when the method is called, the first class (of the method) has already been imported, so importing the second class (which imports the first class) doesn't error. Have you tried this? If this is the real show stopper, we might look into this before restructuring everything. Just a thought.