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

tkfram returns transposed result #473

Closed
fyellin opened this issue Aug 4, 2023 · 5 comments
Closed

tkfram returns transposed result #473

fyellin opened this issue Aug 4, 2023 · 5 comments

Comments

@fyellin
Copy link
Contributor

fyellin commented Aug 4, 2023

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
Copy link
Contributor Author

fyellin commented Aug 4, 2023

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
Copy link
Contributor Author

fyellin commented Aug 5, 2023

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
Copy link
Owner

@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
Copy link
Owner

@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 added a commit that referenced this issue Aug 6, 2023
#474)

* changes tkfram to use official tkfram_c function and adds transpose to zzdynrot to correct order for #473 
* fix for libspicehelper
@AndrewAnnex
Copy link
Owner

fixed by #474

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants