search

sot / Quaternion

Quaternion manipulation
https://sot.github.io/Quaternion
BSD 3-Clause "New" or "Revised" License
3 stars 7 forks source link

Fix for Chandra.Maneuver test failure #19

Closed javierggt closed 4 years ago

javierggt commented 4 years ago

Copying my comment from Issue #18 here:

The problem is in the multiplication. All attributes ere being squeezed to the right shape, but when multiplying two quaternions the result is being inadvertently reshaped, because the result is created from the two input quaternions after applying np.atleast_2d.

One solution is to check the shape of the original quaternions and reshape after the multiplication operation.

While doing this, I realized that there is an implicit broadcasting. We did not think of a generic broadcasting, but the current behavior allows one to do this:

q2 = Quaternion.Quat(equatorial=[[10, 20, 30]])  # shape (1,)
q1 = Quaternion.Quat(equatorial=[1, 2, 3])  # shape ()
q3 = q1 * q2

This is because there is a check that both quaternions have the same shape, but again this is done after np.atleast_2d.

Options:

  1. Forbid the operation above (by requiring the same shape: q1.shape == q2.shape)
  2. Allow this limited broadcasting by setting the output's shape to the largest of the input shapes

Closes #18

javierggt commented 4 years ago

Added fixes and tests for bugs that appeared in proseco regression tests. Now all testr tests in SKA3_HEAD pass.

jeanconn commented 4 years ago

Is this ready for review?

javierggt commented 4 years ago

This has been here for a while.

taldcroft commented 4 years ago

Just to be explicit, you have tested this current version with Chandra.Maneuver and it passes tests?

javierggt commented 4 years ago

Yes, when I made the comment above, I ran testr HEAD tests. Same I am running now.