astropy-healpix icon indicating copy to clipboard operation
astropy-healpix copied to clipboard

High-level API; Remove HEALPix class?

Open cdeil opened this issue 6 years ago • 8 comments

Currently we offer the same functionality via three interfaces (see API).

  1. Functions that use Numpy arrays or Quantity for angles. E.g. lonlat_to_healpix(lon, lat, nside, return_offsets=False, order='ring')
  2. A HEALPix class with (nside, order, frame) data members. One-line methods re-expose functions.
  3. Functions in astropy_healpix.healpy. Drop-in replacements for healpy. To help transition code and with internal testing.

I would like us to discuss and make a decision next week at the workshop on the following questions:

  1. Remove the HEALPix class?
  2. Hide astropy.healpix.healpy from the docs?

Remove HEALPix class?

The HEALPix class (see high_level.py contains nside, order and frame properties, as well as ~ 10 one-line methods re-exposing the function API.

The benefit of the class is that it's very nice for users (see docs):

>>> from astropy_healpix import HEALPix
>>> hp = HEALPix(nside=16, order='nested')
>>> lon, lat = hp.healpix_to_lonlat([1, 442, 2200])
>>> hp.boundaries_lonlat([120], step=1)

A single-class API is nice to import and remember, and only having to pass some parameters once instead of to multiple function calls is convenient.

However, offering the class also has drawbacks:

  • The usage of nuniq indexing is becoming more common, and I think we should support arrays for nside (see #66 #91). This makes it awkward to pass nside in init, it should really be moved to the method calls, leaving only order and frame in init.
  • This also means there is no efficiency benefit of the class, i.e. to pre-compute something for a given nside. We could of course only support scalar nside in the class, and only allow array nside in the functions, like @lpsinger did in #71 .
  • The zen of Python says "there should be one way to do it", and adding the class is just offering a second API to do the same thing, splitting the docs and user base.

I'm +1 to remove the class, and am volunteering to make a PR removing it and rewriting the docs to use the functions.


Hide astropy.healpix.healpy from the docs?

Another question that we should decide before making a PR for astropy.healpix for the Astropy core repo is whether astropy.healpix.healpy should be shown in the public API and docs. At the moment it appears like this: https://astropy-healpix.readthedocs.io/en/latest/healpy_compat.html

I think that's fine, we should keep it as a compatibility and transition layer and for testing, and also mentioning it in the docs like that is OK. But if others think it would be better to hide it more from the docs, or if that is the preference of the healpy maintainers, that seems OK to me as well.


@astrofrog @lpsinger @dstndstn all - please comment!

cdeil avatar Jun 28 '18 09:06 cdeil