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

Make numpy-quaternion optional #519

Merged
merged 28 commits into from
Sep 17, 2024

Conversation

hakonanes
Copy link
Member

@hakonanes hakonanes commented Sep 8, 2024

Description of the change

This PR makes numpy-quaternion an optional dependency. It can be installed using pip install orix[all] or pip install orix numpy-quaternion.

Fixes #514.

I've made a private orix.constants.installed dictionary with (package, installed bool) available. Whether a dependency is installed or not is then checked with installed["numpy-quaternion"].

Functionality previously done by numpy-quaternion can now be done using Numba's generalized universal functions, which handles broadcasting in a smart way. Reduces need for us to check shapes.

How should we handle the optional dependencies on anaconda? I suggest to make an orix-base available, without numpy-quaternion. orix will still have it. What do people think?

Progress of the PR

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 hakonanes added the dev Package maintenance label Sep 8, 2024
@hakonanes hakonanes added this to the v0.13.1 milestone Sep 8, 2024
@hakonanes hakonanes changed the title Make numpy-quaternion and optional dependency Make numpy-quaternion optional Sep 8, 2024
@hakonanes hakonanes marked this pull request as draft September 8, 2024 17:42
@hakonanes
Copy link
Member Author

tomllib requires Python 3.11... Will see if we don't need it. Guess we only have to list optional dependencies in orix.constants.installed.

@hakonanes hakonanes marked this pull request as ready for review September 9, 2024 17:28
@hakonanes
Copy link
Member Author

The failing tests for Python 3.8 are expected and will be addressed in a separate PR addressing #515.

@hakonanes
Copy link
Member Author

hakonanes commented Sep 9, 2024

Wanted to benchmark the re-introduced functionality against numpy-quaternion. In an environment with numpy-quaternion available, I think this is a fair benchmarking of the code used:

import numpy as np
import numba as nb
import quaternion

from orix.quaternion import Quaternion
from orix.vector import Vector3d

n_v = 10
n_qu = 1000
v = Vector3d(np.random.random(n_v * 3).reshape((n_v, 3)))
Q = Quaternion.random(n_qu)


# Called when numpy-quaternion is not available
@nb.guvectorize("(n),(m)->(m)", cache=True)
def qu_rotate_vec_gufunc(
    qu: np.ndarray, v1: np.ndarray, v2: np.ndarray
) -> None:  # pragma: no cover
    a, b, c, d = qu
    x, y, z = v1
    tx = 2 * (c * z - d * y)
    ty = 2 * (d * x - b * z)
    tz = 2 * (b * y - c * x)
    v2[0] = x + a * tx - d * ty + c * tz
    v2[1] = y + d * tx + a * ty - b * tz
    v2[2] = z - c * tx + b * ty + a * tz


@nb.guvectorize("(n),(n)->(n)", cache=True)
def qu_multiply_gufunc(
    qu1: np.ndarray, qu2: np.ndarray, qu12: np.ndarray
) -> None:  # pragma: no cover
    qu12[0] = qu1[0] * qu2[0] - qu1[1] * qu2[1] - qu1[2] * qu2[2] - qu1[3] * qu2[3]
    qu12[1] = qu1[1] * qu2[0] + qu1[0] * qu2[1] - qu1[3] * qu2[2] + qu1[2] * qu2[3]
    qu12[2] = qu1[2] * qu2[0] + qu1[3] * qu2[1] + qu1[0] * qu2[2] - qu1[1] * qu2[3]
    qu12[3] = qu1[3] * qu2[0] - qu1[2] * qu2[1] + qu1[1] * qu2[2] + qu1[0] * qu2[3]


# numpy-quaternion implementation
%timeit \
    qu = quaternion.from_float_array(Q.data);\
    quaternion.rotate_vectors(qu, v.data)
# 69.8 μs ± 571 ns per loop (mean ± std. dev. of 7 runs, 10,000 loops each)
%timeit \
    qu = quaternion.from_float_array(Q.data);\
    qu = quaternion.from_float_array(Q.data);\
    quaternion.as_float_array(qu * qu)
# 4.05 μs ± 35.5 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)

# Alternative implementation
%timeit \
    v2 = np.empty(Q.shape + v.shape + (3,));\
    qu_rotate_vec_gufunc(Q.unit.reshape(-1, 1).data, v.reshape(1, -1).data, v2);\
    v2 = v2.reshape(*Q.shape, *v.shape, 3)
# 92.2 μs ± 988 ns per loop (mean ± std. dev. of 7 runs, 10,000 loops each)
%timeit \
    qu12 = np.empty_like(Q.data);\
    qu_multiply_gufunc(Q.data, Q.data, qu12)
# 37.4 μs ± 170 ns per loop (mean ± std. dev. of 7 runs, 10,000 loops each)

So, numpy-quaternion beats orix with Numba 0.60.0 and NumPy 1.26.4 by:

  • quaternion-vector outer: 1.3x
  • quaternion-quaternion multiplication: 9x

@pc494
Copy link
Member

pc494 commented Sep 11, 2024

Sorry, realise I'm blocking here, plan is to get to it tomorrow/Friday :)

@hakonanes
Copy link
Member Author

That would be great, thanks! Even though all tests pass w/o numpy-quaternion, I get different behavior from Vector3d.get_circle() that I have to fix.

@hakonanes
Copy link
Member Author

Fixed the issue with Vector3d.get_circle(). Just need to update tests and the changelog.

@hakonanes
Copy link
Member Author

Hope to finish testing and changelog updates within Monday. Should be OK to review as is, I don't expect any code changes.

Copy link
Member

@pc494 pc494 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM, couple of tiny things for you to look at and action/ignore @hakonanes.

@@ -13,10 +13,18 @@ With pip
========

orix is availabe from the Python Package Index (PyPI), and can therefore be installed
with `pip <https://pip.pypa.io/en/stable>`__. To install, run the following::
with `pip <https://pip.pypa.io/en/stable>`__.
To install all of orix' functionality, do::
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extremely boring typo, should be orix's

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like typos, means I get to know the language better! I thought words ending with x shouldn't have the s, thank you for pointing this out (:

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The guidance I have found simply says "However, if it sounds better, it is acceptable to just add the apostrophe." so that didn't help, although I would say orix's if I was speaking.

conda activate orix-env

If you prefer a graphical interface to manage packages and environments, you can install
the `Anaconda distribution <https://docs.continuum.io/anaconda>`__ instead.

To install::
To install all of orix' functionality, do::
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe not a typo then?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An intentional typo, if you will

@@ -1237,3 +1239,59 @@ def _outer_dask(
new_chunks = tuple(chunks1[:-1]) + tuple(chunks2[:-1]) + (-1,)

return out.rechunk(new_chunks)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this be obvious if I thought about it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(as in the stuff below this line)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps not. I've added a comment-block to help with the understanding!

except ImportError:
installed[pkg] = False

# Tolerances
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe 1 line here about why we have/want to use two of them (I notice that we do use both)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added an explanation. In general, I'd like for us to use the highest precision possible. That isn't always practical, so we have to lower it, but typically should use either of the two. I guess you could call them "high" and "lower" precisions.

@pc494
Copy link
Member

pc494 commented Sep 17, 2024

Perfect, thanks @hakonanes I'm merging :)

UPDATE: I notice we fail on py3.8 here but I also do not mind (see #515) and think getting this in is more important. I know merging on reds is bad, but I'm making an exception.

@pc494 pc494 merged commit 5367787 into pyxem:develop Sep 17, 2024
10 of 11 checks passed
@hakonanes hakonanes deleted the numpy-quaternion-optional branch September 17, 2024 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dev Package maintenance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make numpy-quaternion an optional dependency
2 participants