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

Refactor the "random" method of Rotations (low-priority) #218

Open
pc494 opened this issue Aug 5, 2021 · 2 comments
Open

Refactor the "random" method of Rotations (low-priority) #218

pc494 opened this issue Aug 5, 2021 · 2 comments
Assignees
Labels
enhancement New feature or request

Comments

@pc494
Copy link
Member

pc494 commented Aug 5, 2021

It currently uses rejection based sampling, I would prefer the method described in:

https://mathworld.wolfram.com/HyperspherePointPicking.html

@pc494 pc494 added the enhancement New feature or request label Aug 5, 2021
@hakonanes
Copy link
Member

I haven't used Rotation.random(), does it not produce the results you would like, is it inefficient, or both?

def random(cls, shape=(1,)):
"""Uniformly distributed rotations.
Parameters
----------
shape : int or tuple of int, optional
The shape of the required object.
"""
shape = (shape,) if isinstance(shape, int) else shape
n = int(np.prod(shape))
rotations = []
while len(rotations) < n:
r = np.random.uniform(-1, 1, (3 * n, cls.dim))
r2 = np.sum(np.square(r), axis=1)
r = r[np.logical_and(1e-9 ** 2 < r2, r2 <= 1)]
rotations += list(r)
return cls(np.array(rotations[:n])).reshape(*shape)

I'm not entierly sure how to implement the equations in the link, but I guess you do. The hypersphere point picking should be added as an alternative method to the existing one, I assume.

@pc494
Copy link
Member Author

pc494 commented Aug 6, 2021

The current version is correct, it's doing the Marsaglia (1972) method from:

https://mathworld.wolfram.com/SpherePointPicking.html

which gives the correct answer. However it's slower than the version I'll replace it with because it involves discarding entries (also it's a while loop).

Consider this nothing more than house keeping that came up while I was working with some of these bits of the code. I'll implement the fix in my big bundle of things.

@pc494 pc494 changed the title Refactor the "random" method of Rotations Refactor the "random" method of Rotations (low-priority) Sep 5, 2021
@pc494 pc494 self-assigned this Sep 5, 2021
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

No branches or pull requests

2 participants