Add test for converting invalid coordinates to an index

[astrofrog]

These should be converted to -1

This test fails for me locally:

    def test_lonlat_to_healpix_invalid():
        # Check that if we pass NaN values for example, the index is set to -1
        ipix = lonlat_to_healpix(np.nan * u.deg, np.nan * u.deg,
                                 nside=1, order='nested')
>       assert ipix == -1
E       assert 8 == -1

astropy_healpix/tests/test_core.py:216: AssertionError

Will be interesting to see if the CI passes.

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (cab333e) 88.27% compared to head (c58fa94) 88.27%.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #207 +/- ## ======================================= Coverage 88.27% 88.27% ======================================= Files 6 6 Lines 503 503 ======================================= Hits 444 444 Misses 59 59 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[astrofrog]

Ok so this is interesting because locally I get:

__________________________________________________ test_lonlat_to_healpix_invalid ___________________________________________________

    def test_lonlat_to_healpix_invalid():
        # Check that if we pass NaN values for example, the index is set to -1
        ipix = lonlat_to_healpix(np.nan * u.deg, np.nan * u.deg,
                                 nside=1, order='nested')
>       assert ipix == -1
E       assert 8 == -1

astropy_healpix/tests/test_core.py:216: AssertionError

Let me see if I can set up a config in CI here that fails.

[astrofrog]

@lpsinger - the armv7 job here can reproduce the failure in the test. A couple of questions:

• What do you think about keeping the exotic archs in the CI? They only take <5 min each, and could be useful at spotting issues? We could even just add armv7 since it reproduces the relevant issue here.

• Do you have any idea how we can try and fix the current failure?

[lpsinger]

@lpsinger - the armv7 job here can reproduce the failure in the test. A couple of questions:

• What do you think about keeping the exotic archs in the CI? They only take <5 min each, and could be useful at spotting issues? We could even just add armv7 since it reproduces the relevant issue here.

Are you able to reproduce this error on your Apple M1 system, as in https://github.com/astropy/reproject/issues/414#issuecomment-1845664231? If so, then I would say no, it's not worth adding these esoteric architectures to the CI/CD pipeline right now. They would take a lot more effort to maintain than platforms that are directly supported by GitHub. And hopefully GitHub Actions will have free tier Apple Silicon runners in the not too distant future.

• Do you have any idea how we can try and fix the current failure?

I don't, but I would try to debug it locally if I could. I would start by tracing the code that is failing the test in GDB or LLDB on an Apple Silicon machine.

[astrofrog]

Yes I can reproduce the test locally. Ok I will remove the CI changes here when at my computer next.

[lpsinger]

Yes I can reproduce the test locally. Ok I will remove the CI changes here when at my computer next.

Well, it would be nice to leave them in a WIP PR so we have them to come back to if we decide otherwise.

[lpsinger]

I think I see the problem. The function that you are testing calls this ufunc loop:

https://github.com/astropy/astropy-healpix/blob/v1.0.1/astropy_healpix/_core.c#L101-L126

which in turn calls this function in the vendored Astrometry.net code:

https://github.com/astropy/astropy-healpix/blob/v1.0.1/cextern/astrometry.net/healpix.c#L900-L904

which calls this function to convert integer pixel indices from one format to another:

https://github.com/astropy/astropy-healpix/blob/v1.0.1/cextern/astrometry.net/healpix.c#L397-L407

Nowhere does it handle invalid indices, though it does assert that the indices are nonnegative --- not sure why those assertions are not crashing us, unless we are always compiling with -DNDEBUG.

I am surprised that this test passes on any platform.

[lpsinger]

See if #208 fixes this.

[astrofrog]

I've rebased this with #208 merged to see if the CI passes. If it does, I'll remove the commits with the CI updates for now and can open another draft PR so we can have those for the record.

[astrofrog]

🎉

[astrofrog]

@lpsinger I am mostly off work this week, could you force push to this PR to remove the CI changes or alternatively cherry pick the test into another PR? Or could just merge this but do a follow up PR to comment out the CI config?

[lpsinger]

@lpsinger I am mostly off work this week, could you force push to this PR to remove the CI changes or alternatively cherry pick the test into another PR? Or could just merge this but do a follow up PR to comment out the CI config?

Sure thing.