High-level API; Remove HEALPix class?

[cdeil]

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 Support arrays for nside #66 Add support for uniq pixel ID scheme #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 Support arrays for nside #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!

[astrofrog]

I don't have much time to comment on this today, but I'm 👎 on removing the class. The intent when writing it was that one day we could implement HEALPix.from_header so that users don't have to manually parse a header and pass the arguments each time to the functions, and it does provide benefits for 'traditional' HEALPix projections.

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 may be true but there are still many files out there that just use the standard HEALPix approach with scalar nside so I don't think it's worth removing the high-level class yet.

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 .

What about simply making it so that it's optional to specify nside when initializing HEALPix and in that case it can be passed during method calls?

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.

Most of the docs actually use the high-level API at the moment. If we want to simplify the API, my personal preference would be to keep only the classes in the public API and hide the individual functions. Most of the docs actually mention the high-level class, it's just the API docs that show the functions so we aren't really splitting the docs.

In addition to making it possible to easily instantiate a HEALPix from a header, the other benefit to me to having a high-level class is that conceptually the class represents a specific tessellation - in a sense this is the same as having a WCS class rather than having to call each WCS method by passing all the WCS parameters each time.

My personal preference is to keep the class approach as the primary way to use the package, and allow nside to optionally be passed during method calls and not during initialization. We can keep the individual functions in the API docs but not mention them anywhere else. If we have to get rid of something I would suggest hiding the healpy functions from the docs. We originally only added that to make it easier to migrate, but I find our main API to be nice and clean and would rather people used that. So the bottom line is that I'd rather get rid of the healpy compatibility layer than our high-level class.

Of course this is just my personal opinion and happy to go with the majority decision.

[astrofrog]

Another benefit of the high-level class is integration with the SkyCoord class, which doesn't exist in the lower-level API.

In any case, having thought about this a bit more, I do agree that the fewer APIs we have to maintain, the better. One option therefore would be to also get rid of the low level API and keep only the high-level API. Just an idea!

[cdeil]

I think we should only offer one API, the functions or the class.

If we choose the class, then I think there's a few options to do it.
One could keep mostly as-is, maybe more the nside or even frame from __init__ to the methods that use it. When using array-valued nside it's weird to pass it in __init__ and the other arguments in the methods.

Another option could be to take more arguments in __init__ but default them all to None, and the same in the methods. Then it's up to the user to only pass them once in __init__ or in each method call. This would be most flexible in terms of usage, but overall I'm not sure flexibility is a win here. I think most users will only call very few methods (usually one or two) and passing a few parameters two times isn't bad, and doesn't offset the complexity / cost of offering mutliple ways to do things. But in the end it's a matter of taste.

This API question would certainly benefit from more user feedback; we could even post it on astropy-dev if we don't get many opinions here.

[astrofrog]

Another option could be to take more arguments in init but default them all to None, and the same in the methods. Then it's up to the user to only pass them once in init or in each method call. This would be most flexible in terms of usage, but overall I'm not sure flexibility is a win here. I think most users will only call very few methods (usually one or two) and passing a few parameters two times isn't bad, and doesn't offset the complexity / cost of offering mutliple ways to do things. But in the end it's a matter of taste.

An intermediate solution is to allow that flexibility only for nside - that is, order/scheme and frame should still be specified in __init__, and we would allow array nside values to be passed during the method call.

I agree that this would likely benefit from user opinions.

[lpsinger]

An intermediate solution is to allow that flexibility only for nside - that is, order/scheme and frame should still be specified in __init__, and we would allow array nside values to be passed during the method call.

I like this option; I'd like to try it out. I am working on some tutorial material on the NUNIQ scheme for our LIGO/Virgo Public Alerts User Guide; this would be a good opportunity to try this out.