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

3 Tests failing on Macos M1 Pro #470

Closed
fyellin opened this issue Jul 26, 2023 · 5 comments
Closed

3 Tests failing on Macos M1 Pro #470

fyellin opened this issue Jul 26, 2023 · 5 comments
Labels
not a bug this issue is not a bug spice question a question that is spice specific

Comments

@fyellin
Copy link
Contributor

fyellin commented Jul 26, 2023

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 AndrewAnnex added spice question a question that is spice specific not a bug this issue is not a bug labels Jul 26, 2023
@AndrewAnnex
Copy link
Owner

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

fyellin commented Jul 27, 2023

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

fyellin commented Aug 2, 2023

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

@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 added a commit that referenced this issue Aug 6, 2023
AndrewAnnex added a commit that referenced this issue Aug 6, 2023
@AndrewAnnex
Copy link
Owner

fixed by #475

AndrewAnnex added a commit that referenced this issue Aug 31, 2023
## [6.0.0] - 2023-08-30
Fixed several major typos and fortran array ordering issues in tkfram, zzdynrot as well as failing tests on arm64 macos. 
### Added
- SpiceNOTENOUGHDATA2 exception #466
- Context manager for SPICE kernels #458
- CITATION.cff file
- DeprecationWarning for zzdynrot

### Changed
- tkfram_c now used in tkfram wrapper function
- updated setup.cfg
- type hints for sincpt to be more correct

### Deprecated
- python 3.6
- python 3.7

### Removed
- codecov as a dependency in dev

### Fixed
- fixed zzdynrot and tkfram return matrix element ordering
- typo in exceptions.rst #459
- fixed test test_sphlat
- fixed sphlat to use correct libspice function call
- fixed tests for dskx02, dskxsi, gfsntc for arm64 macos floating point issues #470
- fixed typo in test_oscelt and test_oscltx
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
not a bug this issue is not a bug spice question a question that is spice specific
Projects
None yet
Development

No branches or pull requests

2 participants