Speed up dot and dot_outer by not calling unique for identical Symmetry

[harripj]

Description of the change

As discussed in #296. The use of unique is slow and causes a significant performance hit when not needed. Currently in Orientation.dot and .dot_outer unique values of the two symmetries are computed, however when the two Symmetry are identical, eg. both Th, then this call to unique is not needed. I suggest we add a check to the function to test whether the two Symmetry are unique, and if so avoid the unique call. This may be further extended to related symmetries, eg. Oh and Th.

Progress of the PR

• [n/a] 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

from orix.quaternion import Orientation, Rotation, symmetry
import numpy as np

o1 = Orientation.random((6, 4))
o2 = Orientation.random((6, 4))

o1.symmetry = symmetry.Th
o2.symmetry = symmetry.Th

def dot(o1, o2):
        """Symmetry reduced dot product of orientations in this instance
        to orientations in another instance, returned as
        :class:`~orix.scalar.Scalar`.
        See Also
        --------
        dot_outer
        """
        symmetry = o1.symmetry.outer(o2.symmetry).unique()
        misorientation = o2 * ~o1
        all_dot_products = Rotation(misorientation).dot_outer(symmetry).data
        highest_dot_product = np.max(all_dot_products, axis=-1)
        return highest_dot_product

def dot_skip_unique(o1, o2):
        """Symmetry reduced dot product of orientations in this instance
        to orientations in another instance, returned as
        :class:`~orix.scalar.Scalar`.
        See Also
        --------
        dot_outer
        """
        if o1.symmetry == o2.symmetry:
                symmetry = o1.symmetry
        else:
                symmetry = o1.symmetry.outer(o2.symmetry).unique()
        misorientation = o2 * ~o1
        all_dot_products = Rotation(misorientation).dot_outer(symmetry).data
        highest_dot_product = np.max(all_dot_products, axis=-1)
        return highest_dot_product

>>> d = dot(o1, o2)
>>> dsu = dot_skip_unique(o1, o2)
>>> np.allclose((d - dsu).angle.data, 0)
True

>>> %timeit dot(o1, o2)
1.77 ms ± 107 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

>>> %timeit dot_skip_unique(o1, o2)
280 µs ± 8.43 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)

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.

[hakonanes]

This is a great speed-up and good additions to the tests.

In addition to the other requests, could you update the changelog?

[harripj]

So an interesting side effect of adding the equality operator to the Rotation class is that Python will no longer hash it, and __hash__ must also be overridden. I propose to revert the last two changes and open a new PR for this.

EDIT
PR is #305.

[harripj]

In fact I believe this can be generalised further by checking whether either Symmetry exists in the subgroup of the other.

all_sym_sg = []

for s1 in symmetry._groups:
    res = []
    for s2 in s1.subgroups:
        # outer of symmetry with its subgroups
        u = s1.outer(s2).unique()
        # assert that unique is same size as main symmetry
        if u.size != s1.size:
            res.append(False)
        else:
            # check that there is no difference between unique
            # and main symmetry
            diff = (s1 * ~u).angle.data
            if np.allclose(diff, 0):
                res.append(True)
            else:
                res.append(False)
    all_sym_sg.append(all(res))
all(all_sym_sg)
True

[pc494]

I've got no comments on this, the coverage has dropped though - happy to let @hakonanes lead reviewing on this.

[harripj]

I've got no comments on this, the coverage has dropped though.

Thanks, I will get on it.

[hakonanes]

The speed-up and much more robusts tests are great!

I have a couple of questions. Also, could you add to the changelog stating that the S4 symmetry has been corrected?

[harripj]

Just a note on performance:

>>> import orix
>>> from orix.quaternion.symmetry import Oh
>>> %timeit Oh == Oh
58.8 µs ± 784 ns per loop (mean ± std. dev. of 7 runs, 10,000 loops each)

>>> %timeit Oh in Oh.subgroups
10 ms ± 124 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

>>> %timeit Oh.outer(Oh).unique()
3.25 ms ± 36.9 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

Checking subgroups is currently detrimental to perfoomance, I will add a boolean parameter to the _get_unique_symmetry_elements function to define whether subgroups are tested. This performance hit is probably due to computation of _differentiators and subgroups at run time. Perhaps these could be computed at instantiation in the future, which may help.

[hakonanes]

Perhaps these could be computed at instantiation in the future, which may help.

We would then have to make Symmetry.data immutable (unchangable). Alternatively, we could try to implement a check that updates subgroups whenever data changes. I don't know how to do such a thing, but it might be worth looking into it.

Checking subgroups is currently detrimental to perfoomance

I assume the timings in the top comment still hold, roughly?

[harripj]

We would then have to make Symmetry.data immutable (unchangable).

This I am okay with, it is essentially the premise with which we implemented the hash. I'm not considering user-created symmetries for now.

I don't know how to do such a thing, but it might be worth looking into it.

I suspect that this feature could be implemented with a setter, at least for the whole array.

I assume the timings in the top comment still hold, roughly?

Yes, for identical symmetries. I think for this PR we can keep this identical symmetry check for the performance benefit. _get_unique_symmetry_elements by default now computes whether the symmetries are identical, and if not explicitly computes the unique() as at the moment this is faster than checking for a subgroup relationship. I have left the subgroup checking code there in the case where we could use it in the future. Otherwise the implemented tests are of benefit for the codebase.

[hakonanes]

I'm not considering user-created symmetries for now

I haven't coded with those in mind either, but functionality should allow a Symmetry instance to be custom, say for example consisting of 3- or 5-fold symmetry operations.

I suspect that this feature could be implemented with a setter, at least for the whole array.

Yes, if the whole data array is updated, this would work. But not if only some elements into data were updated. Then the data.setter wouldn't be called.

This issue gets to the heart of something I struggle to decide on whenever writing a class: whether class properties should be computed upon initialization or created upon request via a property. The latter provides for more flexibility, but comes at a computational cost. Unless it is performance critical, I usually opt for this approach.

[hakonanes]

I'm very happy with this PR, and I'll leave it up to @pc494 to review and merge when he finds the time.

[pc494]

LGTM, merging