AndrewAnnex / SpiceyPy

SpiceyPy: a Pythonic Wrapper for the SPICE Toolkit.
MIT License
384 stars 84 forks source link

tkfram returns transposed result #473

Closed fyellin closed 1 year ago

fyellin commented 1 year ago

tkfram returns the incorrect rotation vector. The vector it returns is the transpose of the correct value.

I copied the test from the tkfram_c documentation and performed a straightforward conversion of it into Python. When I run the test as given, it fails. When I uncomment the line rotation = rotation.T, it passes. See below.

I'm not quite sure where in the SpiceyPy code the array is being transposed. Does it think it's getting back a Fortran array rather than a C array? The implementation of tkfram_c already performs the transposition.

I have not yet checked if this bug is only in tkfram, or if it occurs in other rotation matrices.

import pytest
import spiceypy as cs

def test_tkfram():
    cs.furnsh("kernels/earth_topo_050714.tf")
    frcode = cs.namfrm("DSS-34_TOPO")
    rotation, frame = cs.tkfram(frcode)
    frname = cs.frmnam(frame)
    assert frname in ("IAU_EARTH", "EARTH_FIXED", "ITRF93")
    # rotation = rotation.T
    z = np.array((rotation[0, 2], rotation[1, 2], rotation[2, 2]))
    radius, lat, lon = cs.reclat(z)
    assert pytest.approx(lat * cs.dpr()) == 148.9819650021110
    assert pytest.approx(lon * cs.dpr()) == -35.3984778756552
fyellin commented 1 year ago

I just noticed that the implementation of tkfram calls libspice.tkfram_ instead of libspice.tkfram_c. If it is calling the Fortran function rather than the C function, it will get back the array transposed.

Is this intentional or a typo?

fyellin commented 1 year ago

A quick glance at the code. irfrot, irftrn, and zzdynrot all expect to receive a 2-dimensional vector directly from Fortran. I have not tested yet, but I suspect all three will give transposed arrays as their result.

AndrewAnnex commented 1 year ago

@fyellin tkfram, as I recall, was added to the spiceypy api before a _c version of the function was provided by the NAIF (as of N67, IIRC) and just hasn't been updated yet. The transposes of the arrays is troubling so I will look into that

AndrewAnnex commented 1 year ago

@fyellin okay so this may be because tkfram, irfrot, irftrn, and zzdynrot were all calling the fortran versions of the functions. Some of these functions explicitly transpose the results like irftrn and irfrot, but not always like for tkfram which may be a mistake Overall there seem to be 16 functions from the fortran api wrapped in spiceypy but most of these do not return arrays where this ordering becomes important.

AndrewAnnex commented 1 year ago

fixed by #474