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

Avoid expensive deepcopy when rotating Miller #533

Merged
merged 1 commit into from
Dec 15, 2024

Conversation

viljarjf
Copy link
Contributor

@viljarjf viljarjf commented Dec 10, 2024

Description of the change

Rotating a Miller instance with a Rotation calls Miller.__getitem__ to handle improper rotations, regardless of whether any rotations are actually improper.
As Miller.__getitem__ involves a deepcopy, avoiding this when not necessary provides a significant speedup, depending on the complexity of the associated Phase.

Progress of the PR

Minimal example of the bug fix or new feature

from orix.crystal_map import Phase
from orix.vector import Miller
from orix.quaternion import Rotation

P1 = Phase("P1", 1)
Fm3m = Phase("Fm3m", 225)
P1_vec = Miller.random(P1)
Fm3m_vec = Miller.random(Fm3m)
rot = Rotation.random(2000)


%timeit rot * P1_vec
%timeit rot * Fm3m_vec

Current
505 μs ± 18.6 μs per loop (mean ± std. dev. of 7 runs, 1,000 loops each)
1.7 ms ± 48.2 μs per loop (mean ± std. dev. of 7 runs, 1,000 loops each)

New change
346 μs ± 41.4 μs per loop (mean ± std. dev. of 7 runs, 1,000 loops each)
350 μs ± 16.9 μs per loop (mean ± std. dev. of 7 runs, 1,000 loops each)

Of course, with improper rotations, the runtime is the same:

from orix.crystal_map import Phase
from orix.vector import Miller
from orix.quaternion import Rotation

P1 = Phase("P1", 1)
Fm3m = Phase("Fm3m", 225)
P1_vec = Miller.random(P1)
Fm3m_vec = Miller.random(Fm3m)
rot = Rotation.random(2000) * -1

assert all(rot.improper)

%timeit rot * P1_vec
%timeit rot * Fm3m_vec

506 μs ± 26.3 μs per loop (mean ± std. dev. of 7 runs, 1,000 loops each)
1.69 ms ± 44.3 μs per loop (mean ± std. dev. of 7 runs, 1,000 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.
  • Contributor(s) are listed correctly in __credits__ in orix/__init__.py and in
    .zenodo.json.

@hakonanes hakonanes self-requested a review December 10, 2024 11:37
@hakonanes
Copy link
Member

Seems simple enough. I'll have a look within tomorrow.

@viljarjf viljarjf mentioned this pull request Dec 10, 2024
11 tasks
@hakonanes
Copy link
Member

Looking beyond this PR, I think it may be better to avoid deepcopy() altogether and rather add a copy() method, like this

def copy(self) -> Self:
    return self.__class__(self)

We can update the creation method to allow another instance as the first parameter, and only copy the things we need.

We can deprecate deepcopy() in favor of copy().

Benefits:

  • More control
  • New copy via Vector3d(Vector3d) or Vector3d.copy()

What do you think, @viljarjf?

@hakonanes hakonanes added the enhancement New feature or request label Dec 15, 2024
@hakonanes hakonanes added this to the v0.14.0 milestone Dec 15, 2024
@hakonanes hakonanes merged commit 8ac149f into pyxem:develop Dec 15, 2024
12 checks passed
@hakonanes
Copy link
Member

Let's continue the copy discussion in #535.

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

Successfully merging this pull request may close these issues.

2 participants