hipspy / hips

Python library to handle HiPS
https://hips.readthedocs.io
12 stars 16 forks source link

Change from healpy to astropy-healpix #109

Closed cdeil closed 6 years ago

cdeil commented 6 years ago

This is a first quick attempt to change the hips package from healpy over to the to-be Astropy healpix package (http://healpix.readthedocs.io/). The main motivation for the change is to support Windows, another motivation is to have an easier-to-install dependency (or no dependency at all if this gets merged as astropy.healpix into the core at some point).

This pull request is work in progress, it should only be merged after there has been a release and a conda package exists for the Astropy healpix package.

In addition, I'd like to remove hips/utils/healpix.py and change the callers to use the main Astropy healpix API, not the healpy-compatible API, and the dependency declaration in setup.py as well as the install docs need to be updated.

@astrofrog - For now, I see two issues when running the tests: https://gist.github.com/cdeil/a81421e94df6ed4cd732ddcf61c4174a

  1. We are passing 2-dim arrays, but currently only 1-dim arrays are supported. This is issue https://github.com/cdeil/healpix/issues/21 . This is a blocker for making the change.
  2. We are calling healpy.boundaries which is still missing in healpix.healpy. This is issue https://github.com/cdeil/healpix/issues/23 . This means it's not a drop-in replacement, but we should change to healpix_boundaries_lonlat anyways, so not really needed.
astrofrog commented 6 years ago

This is a first quick attempt to change the hips package from healpy over to the to-be Astropy healpix package (http://healpix.readthedocs.io/). The main motivation for the change is to support Windows, another motivation is to have an easier-to-install dependency (or no dependency at all if this gets merged as astropy.healpix into the core at some point).

Also licensing :)

@cdeil - ok, we should milestone cdeil/healpix#21 as 0.1. Do you have any time to work on this? Unfortunately I have to put work on healpix on hold for a few days while catching up with some other projects, so if you would have any time to work on it that would be great.

cdeil commented 6 years ago

@astrofrog - I'm afraid I won't have time for https://github.com/cdeil/healpix/issues/21 this week. I'll take a quick stab at implementing https://github.com/cdeil/healpix/issues/31 (much simpler, smaller task) now, but probably won't even have time to finish that (i.e. add tests covering all edge cases, checking the maximum resolution we can support).

cdeil commented 6 years ago

Locally now hipspy works with astropy-healpix. Let's see if we can make it work on the CI builds, especially Appveyor (Windows never worked until now, but now it should).

cdeil commented 6 years ago

@astrofrog - Any idea why the Appveyor build tried to pip install scipy? https://ci.appveyor.com/project/cdeil/hips-19275o3fsp6drby/build/1.0.336

I'll change Appveyor to Python 3.6 now, but I presume the scipy install fail will still be there.

cdeil commented 6 years ago

It was because of scikit-image. Now CI should pass. I'll check back tomorrow.

cdeil commented 6 years ago

First successful build / test on Windows: https://ci.appveyor.com/project/cdeil/hips-jra0zyeh9itvq3c/build/1.0.338

coveralls commented 6 years ago

Coverage Status

Coverage remained the same at 96.733% when pulling dba6482e623c2a7847a760c1506c69449b92d58d on cdeil:astropy-healpix into ed7d02d979a5e4966efad509184e942ae487b160 on hipspy:master.

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-2.9%) to 93.863% when pulling d633e183583d9f8495ab4cfdb5bd598d028104a7 on cdeil:astropy-healpix into ed7d02d979a5e4966efad509184e942ae487b160 on hipspy:master.

cdeil commented 6 years ago

There is this one fail on Windows: https://ci.appveyor.com/project/cdeil/hips-19275o3fsp6drby/build/1.0.340#L316

I think it's an int overflow. I'll try adding a dtype=float to the np.sum call - that should resolve the issue by using a float64 accumulator.

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-1.3%) to 95.397% when pulling 66299cd1bab889ad1839f8d0978c5f199d54f333 on cdeil:astropy-healpix into ed7d02d979a5e4966efad509184e942ae487b160 on hipspy:master.

cdeil commented 6 years ago

Yippie!

astrofrog commented 6 years ago

Nice! 🎉