Multi-order coverage MOC HEALPIX

cdeil commented 7 years ago

It would be nice to have Multi-order coverage (MOC) in this astropy.regions package (which is supposed to go in the Astropy core package in a few month).

http://ivoa.net/documents/MOC/ describes it like this:

This document describes the Multi-Order Coverage map method (MOC) to specify arbitrary sky regions. The goal is to be able to provide a very fast comparison mechanism between coverage maps. The mechanism is based on the HEALPix sky tessellation algorithm. It is essentially a simple way to map regions of the sky into hierarchically grouped predefined cells.

There's two Python package for MOC:

pymoc by @grahambell - Github, Readthedocs, PyPI
mocpy by @tboch - Github, example notebooks, PyPI

@grahambell @tboch - Are there big differences in design or philosophy between the two Python packages? Or does collaboration / merging of efforts seem possible? Are you interested to collaborate in that direction?

If there is interest in making this happen, one issue to watch out for is license. Both pymoc and mocpy are currently implemented using healpy, which uses HEALPIX and is GPL licensed, not compatible with the BSD license Astropy core and most other packages use. There is talk of having a subset of HEALPIX re-implemented in BSD and use that, starting with the BSD-licensed HEALPIX code e.g. here or here and here. I think having such basic HEALPIX functionality in Astropy core and using it for MOC and other applications would be nice.

@astropy/regions-developers - Agreed that MOC would be in scope and nice to have for astropy.regions
@grahambell @tboch - Do you support this? Or would you prefer your Python MOC packages stay as-is (not integrated in Astropy, GPL licensed)?
Anyone besides me have time / interest to work in this in the coming weeks or months?

tboch commented 7 years ago

Hi @cdeil

I just discovered this thread. CDS and I are very much in favour of integrating MOC support into Astropy regions. (with a BSD license) So, yes, I would be interested to collaborate on this, and perhaps also on the reimplementation of HEALPix functions.

cdeil commented 7 years ago

I've talked with @tboch a bit about this on email today, and we both think (and are willing to put some work into) a BSD-licensed healpix Python package and then use it here in regions for MOC and in other packages (e.g. Gammapy, see here).

As discussed before, the best starting point seems to write a wrapper on the C healpix package by @dstndstn in astrometry.net (see e.g. https://github.com/dstndstn/astrometry.net/blob/master/include/astrometry/healpix.h) that's BSD license, so appropriate for use in Astropy.

@dstndstn @astrofrog and all -- do you have thoughts preferences on how to best do this?

@dstndstn - Is it obvious which files to extract from astrometry.net or is it a ton of files and complicated because there's many includes?
Cython wrapper with healpy-compatible API OK? I don't have much Cython experience, but I'd like to try this. Of course, other proposals to wrap differently or move some code to Python / Numpy are welcome.
Do it in this repo and expose it for now as regions.healpix or start a new one?
Does anyone have a need (and is willing to put some effort into) a standalone C package, i.e. a C build system (Makefile, autotools, whatever you like) that are separate from the setup.py based build?

astrofrog commented 7 years ago

I would say keep it in a separate repo. If anyone does have the time to maintain a C package/library, that would be ideal, but if not then having a Python package is fine (and maybe later the C can be split out if it seems it would be popular enough). Cython is fine.

dstndstn commented 7 years ago

I have pythonic swig wrappers on some of my healpix routines.

It should be fairly clean to pull out the healpix routines -- the functions are in -- util/healpix.c, util/healpix-utils.c, and they include a few others modules. Only unusual thing is "os-features", which handles a couple of functions that are handled differently in different OSes. I bet it could be removed, and I'd be willing to do that work.

cheers, --dustin

On Thu, Sep 1, 2016 at 2:05 PM, Thomas Robitaille notifications@github.com wrote:

I would say keep it in a separate repo. If anyone does have the time to maintain a C package/library, that would be ideal, but if not then having a Python package is fine (and maybe later the C can be split out if it seems it would be popular enough)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/astropy/regions/issues/62#issuecomment-244212353, or mute the thread https://github.com/notifications/unsubscribe-auth/ABBD_cv9bdRu4d7bTEQSPZJwOY7jYJKvks5qlz4ugaJpZM4JV7Fi .

cdeil commented 7 years ago

I took a quick stab at extracting it into a C lib: https://github.com/cdeil/healpix-temp

After I added os-features.h it's asking for os-features-config.h, which I can't find in the astrometry.net repo (maybe auto-generated?).

@dstndstn - Yes, if you could remove the dependency on os-features, that would be very helpful.

Also - do your SWIG Python wrappers provide a Python API that can take Numpy arrays as input / output like healpy does? Even if yes, do you think they should be kept or would replacing them with Cython be OK?

I'm asking because Astropy and the affiliated packages use Cython a lot, but no-where SWIG (as far as I know). So for maintenance / distribution for us Cython would be easier.

dstndstn commented 7 years ago

Hi,

Yes, os-features-config.h is auto-generated (by the Makefile). I can take a shot at removing that dependency when I'm back from my long weekend away.

The swig wrappers I have are pretty simple and do not support numpy arrays; cython would be fine.

Would you like me to send a PR to that healpix-temp repo, if you've already started extracting code?

cheers, --dustin

On Thu, Sep 1, 2016 at 11:50 PM, Christoph Deil notifications@github.com wrote:

I took a quick stab at extracting it into a C lib: https://github.com/cdeil/healpix-temp

After I added os-features.h it's asking for os-features-config.h, which I can't find in the astrometry.net repo (maybe auto-generated?).

@dstndstn https://github.com/dstndstn - Yes, if you could remove the dependency on os-features, that would be very helpful.

Also - do your SWIG Python wrappers provide a Python API that can take Numpy arrays as input / output like healpy does? Even if yes, do you think they should be kept or would replacing them with Cython be OK?

I'm asking because Astropy and the affiliated packages use Cython a lot, but no-where SWIG (as far as I know). So for maintenance / distribution for us Cython would be easier.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/astropy/regions/issues/62#issuecomment-244297115, or mute the thread https://github.com/notifications/unsubscribe-auth/ABBD_famPk6iSl8g_WsWcefuK6YzzgAKks5ql8c0gaJpZM4JV7Fi .

cdeil commented 7 years ago

@dstndstn - Sounds great, thanks!

Actually, if you could put the files in a sub-folder in this repo that would be best: https://github.com/cdeil/healpix I've already set up travis-ci there and that's the one we'll use for the near future and then the commit history will reflect that this is your code.

I'll give you and @tboch write access to that repo now, and tomorrow update the README there to reflect the new plan (Cython wrapper around your code).

cdeil commented 7 years ago

I've now put together the initial version of the package: https://github.com/cdeil/healpix

There's a few setup issues with Cython and Sphinx that need to be sorted out: https://github.com/cdeil/healpix/issues Help with those is very welcome!

@dstndstn @tboch - Let's continue the discussions on HEALPix there for a while until the basic functionality is in place, and then continue in this issue to discuss MOC.

cdeil commented 7 years ago

Status update: @dstndstn added the astrometry.net HEALPIX C library code in https://github.com/cdeil/healpix/pull/6

At the moment it's here, can be compiled with make and C tests run: https://github.com/cdeil/healpix/tree/master/astrometry.net

Next steps are to build from setup.py, then wrap with Cython, add tests.

cdeil commented 6 years ago

This is now pretty far along: http://astropy-healpix.readthedocs.io/

@tboch - If you're up for it, we could make the switch soon?

I had a look, the only functionality that's missing is that you're calling healpy.mollview and healpy.graticule in MOC.plot. @astrofrog - Are there any plans to add HEALPix visualisation helper functions or classes to astropy-healpix? Or maybe consider adding those two to the astropy_healpix.healpy compat interface?

Before we do though: it would be really helpful to add a few tests for the main functionality in mocpy. I started in https://github.com/cds-astro/mocpy/pull/6 but I'm not very familiar with MOC. @tboch - maybe you could add some tests to mocpy?

Also: @tboch - Are there any performance concerns? This is what we have at the moment: http://astropy-healpix.readthedocs.io/en/latest/performance.html Is this good enough for mocpy? If not - can you share a test case or two that are currently too slow?

astrofrog commented 6 years ago

I haven't used healpy.mollview or healpy.graticule, so I don't have a good sense for how much work they would be to implement. Can you show an example of output of these functions?

tboch commented 6 years ago

@cdeil : healpy.mollview and healpy.graticule would be nice to have, but it's not at the core of MOCPy features.

I'll make some performance tests with astropy-healpix and will let you know the results.

And yes, I'll add tests.

astropy / regions

Multi-order coverage MOC HEALPIX #62