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

rotation._compute_euler_from_matrix() creates an array with negative stride (-8) #13584

Closed
marscher opened this issue Feb 18, 2021 · 5 comments · Fixed by #13598
Closed

rotation._compute_euler_from_matrix() creates an array with negative stride (-8) #13584

marscher opened this issue Feb 18, 2021 · 5 comments · Fixed by #13598
Labels
maintenance Items related to regular maintenance tasks scipy.spatial
Milestone

Comments

@marscher
Copy link
Contributor

marscher commented Feb 18, 2021

The scipy.spatial.transform.Rotation class generates a "weird" output array when calling the method as_euler. The stride of this array is negative (-8). This does not seem like a problem, but causes issues in downstream software, e.g. when serializing the array.
Taking a copy "fixes" the stride again, e.g. makes it positive again. I assume that this is caused by the extrensic broadcasting/reversing done here:

angles = angles[:, ::-1]

Downstream code using Rotation.from_euler worked prior version 1.6, in which the Rotation class is a cythonized class.

xref: asdf-format/asdf#916
xref: BAMWelDX/weldx#226

Reproducing code example:

from scipy.spatial.transform import Rotation

R = Rotation([1, 0, 0, 0])
euler = R.as_euler("xyz")
assert all(i >= 0 for i in euler.strides), euler.strides
euler_intrinsic = R.as_euler("XYZ")
assert all(i >= 0 for i in euler_intrinsic.strides), euler.strides # this "works", e.g. has a positive stride.

Error message:

    assert all(j >= 0 for i,j  in enumerate(euler.strides)), euler.strides
AssertionError: (-8,)
  ...

Scipy/Numpy/Python version information:

1.6.0 1.20.1 sys.version_info(major=3, minor=7, micro=9, releaselevel='final', serial=0)

@marscher
Copy link
Contributor Author

Mhm I guess this is not a bug, but quite expected due to the reversing array operation. So downstream projects should deal with this, right?

@tylerjereddy
Copy link
Contributor

Let's check with @nmayorov

@nmayorov
Copy link
Contributor

@marscher

To be honest I have never thought about array strides and especially negative strides. It looks like having negative strides is strange and unnecessary. Adding copy operation easily fixes that:

a = np.arange(10)
b = a[::-1]
print(b.strides)

b = a[::-1].copy()
print(b.strides)
(-8,)
(8,)

Can you send a fix PR for that?

@marscher
Copy link
Contributor Author

marscher commented Feb 21, 2021 via email

marscher added a commit to marscher/scipy that referenced this issue Feb 22, 2021
…ut for output array

The output array (or memoryview) used to have a negative stride, which may cause problems later on when trying to
wrap it in an ndarray again.

Fixes scipy#13584
marscher added a commit to marscher/scipy that referenced this issue Feb 22, 2021
The output array (or memoryview) used to have a negative stride,
which may cause problems later on when trying to wrap it in an ndarray again.
This is only the case for extrinsic rotations.

Closes scipy#13584
@marscher
Copy link
Contributor Author

@nmayorov I've opened up PR #13598

@tylerjereddy tylerjereddy added this to the 1.7.0 milestone Feb 26, 2021
@tylerjereddy tylerjereddy added the maintenance Items related to regular maintenance tasks label Feb 26, 2021
@tylerjereddy tylerjereddy modified the milestones: 1.7.0, 1.6.2 Mar 20, 2021
tylerjereddy pushed a commit to tylerjereddy/scipy that referenced this issue Mar 20, 2021
The output array (or memoryview) used to have a negative stride,
which may cause problems later on when trying to wrap it in an ndarray again.
This is only the case for extrinsic rotations.

Closes scipy#13584
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Items related to regular maintenance tasks scipy.spatial
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants