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

Added failing example for test_pix2ang #84

Merged
merged 1 commit into from
Jul 3, 2018

Conversation

astrofrog
Copy link
Member

@lpsinger - #80 is not related to PPC, it is just a failure that happened to appear in that build. This PR adds that specific example to the hypothesis test and this fails for me locally on Mac. I don't have time to investigate why this is happening, so if anyone has any time please feel free to!

@astrofrog
Copy link
Member Author

This fails on Travis, as expected (I think healpy doesn't get installed on AppVeyor)

@cdeil
Copy link
Member

cdeil commented Jun 24, 2018

Here's a small example to reproduce the issue:

import numpy as np
import astropy_healpix

nside = 2 ** 27
frac = 2 / 3
ipix = int(frac * 12 * nside ** 2)
healpix = astropy_healpix.HEALPix(nside, order='nested')
lon, lat = healpix.healpix_to_lonlat(ipix)
phi = lon.rad
theta = np.pi / 2 - lat.rad

print('nside:', nside)
print('ipix=', ipix)
print('lat.dtype=', lat.dtype)
print('phi=', phi)
print('lat.deg=', lat.deg)
print('distance_to_south_pole=', np.pi - theta)

The problem is that this pixel is placed directly at the south pole:

nside: 134217728
ipix= 144115188075855872
lat.dtype= float64
phi= 0.0
lat.deg= -90.0
distance_to_south_pole= 0.0

healpy places is (presumably correctly) above the south pole

import healpy as hp
theta, phi = hp.pix2ang(nside, ipix, nest=True)
print(theta.dtype)
print('phi=', phi)
print('distance_to_south_pole=', np.pi - theta)
float64
phi= 0.7853981633974483
distance_to_south_pole= 6.083373360610267e-09

@dstndstn - could you please try to run this example through only your C code? Is it a bug there, or is it a problem that's introduced in the Python wrapper? (e.g. because of float or int precision / size or rounding issues?)

@cdeil
Copy link
Member

cdeil commented Jun 24, 2018

@astrofrog - I now see #34 #38 . Looks like this case is just an example of a known issue: precision near the poles.

@cdeil cdeil added this to the 0.3 milestone Jun 24, 2018
@cdeil cdeil mentioned this pull request Jun 26, 2018
@cdeil
Copy link
Member

cdeil commented Jun 26, 2018

I restarted CI after merging #94.

Looks like the phi is still incorrect for this test case?
https://travis-ci.org/astropy/astropy-healpix/jobs/381277304#L1064

So @dstndstn @mreineck - looks like there are other precision issues in the C code.

@dstndstn
Copy link
Collaborator

dstndstn commented Jun 27, 2018 via email

@astrofrog
Copy link
Member Author

astrofrog commented Jun 27, 2018

I often have issues with pip for healpy and have to resort to using the conda package from conda-forge.

@dstndstn
Copy link
Collaborator

I looked into this failing test case. It's very similar, in that there is cancellation error near the pole.

Specifically, the term being squared here is ~1e-9, so z -> 1 (cancellation error; it should be 1 - 1e-18).
https://github.com/astropy/astropy-healpix/blob/master/cextern/astrometry.net/healpix.c#L1013

@dstndstn
Copy link
Collaborator

I've got a fix for this

@cdeil
Copy link
Member

cdeil commented Jun 28, 2018

@dstndstn - If you're willing to use conda, there's now a quick way to get an environment with healpy and everything you need for hacking on astropy-healpix: a0b72e7

My experience is that pip install healpy sometimes works and sometimes give linker errors like yours, but using conda and the healpy from conda-forge works reliably.

@cdeil
Copy link
Member

cdeil commented Jun 28, 2018

I've got a fix for this

Great! Please link to this issue when you make a pull request.

@dstndstn
Copy link
Collaborator

Could you please check whether #101 fixes this?
I'm going to be away this weekend.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 96.214% when pulling ed9305b on astrofrog:test-pix2ang into a0b72e7 on astropy:master.

@astrofrog
Copy link
Member Author

I've rebased this, so it can be merged if CI passes

@cdeil
Copy link
Member

cdeil commented Jul 3, 2018

Looks like this case is working now. @dstndstn @astrofrog - Thanks!

@cdeil cdeil merged commit 528292b into astropy:master Jul 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants