SyneRBI / SIRF-Exercises

SIRF Training and demonstration material
http://www.ccpsynerbi.ac.uk
Apache License 2.0
18 stars 21 forks source link

Geometry notebook has wrong RAH2LPH matrix #136

Closed johannesmayer closed 3 years ago

johannesmayer commented 3 years ago

@DANAJK hello David, you use the matrix in your notebook

RAH2LPH = [[-1, -1, -1, -1 ],
                     [-1, -1, -1, -1 ],
                     [ 1,  1,  1,  1 ],
                     [ 1,  1,  1,  1 ]]

to transform between LPH and RAH coordinates, however, this matrix is not invertible and hence can not be used to transform between the systems. The correct one is (should be) this one, where you revert the first two coordinate directions:

RAH2LPH = [[-1, 0, 0, 0 ],
           [ 0,-1, 0, 0 ],
           [ 0, 0, 1, 0 ],
           [ 0, 0, 0, 1 ]]

and then we need to use either

A_LPH = np.matmul(RAH2LPH, A_RAH) instead of A_LPH = np.mult(A_RAH, RAH2LPH) or A_LPH = np.matmul(RAH2LPH, np.matmul(A_RAH, np.transpose(RAH2LPH))) afterwards (not sure which one yet). The order is important, before this non-invertible RAH2LPH just commuted.

johannesmayer commented 3 years ago

I think A_LPH = np.matmul(RAH2LPH, A_RAH) produces the same results below in the notebook as happened with your other matrix. Since the indices are coming in the RAH system already, the A_LPH = np.matmul(RAH2LPH, np.matmul(A_RAH, np.transpose(RAH2LPH))) would be wrong, that's just the transform if you want to apply it to LPH indices.

I will make a PR.

KrisThielemans commented 3 years ago

well spotted!