AndrewAnnex / SpiceyPy

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

3 Tests failing on Macos M1 Pro #470

Closed fyellin closed 1 year ago

fyellin commented 1 year ago

Describe the bug Three tests in test_wrapper.py are failing when I run them on my MacOS M1.
test_dskx02 test_dskxsi test_gfsntc

For the first two, these are because test is expecting a specific right answer when there are, in fact, multiple possible right answers. For the third, I am getting a difference in the last bit of the answer, but the test is doing exact string comparison.

test_dskx02 and test_dskxsi: I actually emailed Nate Bachman who wrote the Spice DSK subsystem about this. (I originally presumed this was a cspyce problem. But I know see the SpiceyPy has the same issue). The tests ask for the closest triangle to a point many kilometers above the surface. It turns out that the closest point is a vertex, and that multiple triangles share this vertex.

Look at the output dskexp -dsk phobos_lores.bds -text phobos_lores.txt -format plate-vertex -prec 10. You can see that the closest point is vertex 194, and that any triangle that includes vertex 194 (viz. 349, 350, 420, 421, 422, 423) is a valid response.

test_gfsntc On my machine, I'm getting the result "2007-SEP-23 09:46:39.606981 (TDB)", which differs in the last significant digit from the expected result. Both spiceypy and cspyce give the same "incorrect" result, so I'm sure that the problem is a small difference in the math calculation.

To Reproduce I simply run the test on my MacOs.

Desktop (please complete the following information): spiceypy version: 5.1.2 python version 3.10.2 Macos Version. 13.3.1(a) Ventura Macintosh Apple M1 Pro

AndrewAnnex commented 1 year ago

Yeah this looks to be a difference perhaps in how the code is compiled on ARM vs x86. Interestingly this is not a bug observed in the CI/CD because of the fact the environments are emulated, macos ARM builds are not tested in the infrastructure (although aarch64 test are run). I don't think this is a spiceypy bug per-say, as it's non unique to spiceypy, but rather is a bug or deficiency of cspice itself. I only recently purchased a new apple computer so I haven't developed any new spiceypy code on it directly yet also reducing my visibility.

It sounds like then for dskx02 and dskxsi that the test should be updated to accept any of those answers or assert that 420 is in the ic result if it contains all of those values, although it is maybe disturbing that the same result is not returned for x86/arm.

The timing thing I'll want to look into but maybe this is a floating point difference between arm/x86 so I'm not sure what the correct thing to do about that.

fyellin commented 1 year ago

I agree that there isn't a bug SpiceyPy, but rather in the test suite.

And yes, there is clearly a subtle mathematical difference happening somewhere. I tried the following on my machine, running both native ARM code and emulated x86_64 code. This is the essence of the dskxsi test. No cspyce, no spiceypy, no cspice.so.

% cp /tmp/main.c  .
% cp /tmp/phobos_lores.bds  .
% curl https://naif.jpl.nasa.gov/pub/naif/misc/toolkit_N0067/C/MacM1_OSX_clang_64bit/packages/cspice.tar.Z | gzip -d > cspice.tar
% tar xf cspice.tar
% cc -arch arm64 -Icspice/include/ main.c cspice/src/cspice/*.c -o main
% ./main
found = 1
dc = [0.000000]
ic = [423]

% cc -arch x86_64 -Icspice/include/ main.c cspice/src/cspice/*.c -o main
% ./main
./main
found = 1
dc = [0.000000]
ic = [420]

main.c:

#include "SpiceUsr.h"
#include <stdio.h>

int main(int c, char **string) {
    ConstSpiceInt srflst[] = { 401 };
    ConstSpiceDouble vertex[3] = { 1e10, 0, 0 };
    ConstSpiceDouble raydir[3] = {-1e10, 0, 0 };
    SpiceDouble xpt[3];
    SpiceInt handle;
    SpiceDLADescr dladsc;
    SpiceDSKDescr dskdsc;
    SpiceDouble dc[1];
    SpiceInt ic[1];
    SpiceBoolean found;

    furnsh_c("phobos_lores.bds");
    dskxsi_c(0, "PHOBOS", 1, srflst, 0.0, "IAU_PHOBOS", vertex, raydir, 1, 1,
             xpt, &handle, &dladsc, &dskdsc, dc, ic, &found);

    printf("found = %d\n", found);
    printf("dc = [%f]\n", dc[0]);
    printf("ic = [%d]\n", ic[0]);
    return 0;
}
fyellin commented 1 year ago

Andrew, do you want me to create a pull request with new tests for these three? Or should we wait for a more comprehensive solution from Spice?

AndrewAnnex commented 1 year ago

@fyellin I feel like the discussion hasn't completed yet on that email chain, but I suspect the answer will be to just make the test requirements less precise and adding those other plate id's as acceptable answers rather than adding platform specific conditions or new tests. Unfortunately preoccupied with work so won't get to this or the other issues on the repo until the weekend

AndrewAnnex commented 1 year ago

fixed by #475