xarray-contrib / xdggs

Xarray extension for DGGS
https://xdggs.readthedocs.io
Apache License 2.0
80 stars 14 forks source link

replace `healpy` with `cdshealpix` #92

Closed keewis closed 1 week ago

keewis commented 2 weeks ago

It seems that by importing healpy xdggs may be violating the license of healpy (GPLv2+): as far as I can tell, all "strong" GPL licenses consider libraries dynamically linking – in python: importing – to other libraries as "derived" from the latter, and thus must also be GPL. The version of GPL that would allow importing in a non-GPL library is LGPL, but that's not what healpy chose as license.

Since I really don't want to re-license xdggs to GPLv2+, this PR replaces healpy with cdshealpix-python (BSD-3) and cdshealpix-rust (Apache-2), both of which are compatible with our current license.

This also allows us to run a windows CI, since healpy was the library that prevented us from supporting it (however, it seems cdshealpix is still not available for macos arm on conda-forge).

cc @tinaok, @annefou, @benbovy, @allixender

tinaok commented 2 weeks ago

@zawadzl

benbovy commented 2 weeks ago

it seems cdshealpix is still not available for macos arm on conda-forge

They seem to be working on it (recent open PRs on the feedstock repo).

keewis commented 2 weeks ago

They seem to be working on it (recent open PRs on the feedstock repo).

Thanks for the heads-up. That might still take a while, though (last activity was 3 weeks ago). I'll try to work around this until those PRs are merged by using the wheels on PyPI (similar to what I did for lonboard). No idea why the windows CI fails, though.

benbovy commented 2 weeks ago

Did you also consider https://github.com/astropy/astropy-healpix (BSD-licensed C wrapper) as an alternative? It has packages for all platforms on conda-forge.

keewis commented 2 weeks ago

I had seen it, but didn't consider it, strangely.

Edit: I don't think that is a C wrapper, though, it uses a different codebase (astrometry.net?)

Both that and cdshealpix-rust have about the same features, but while astropy-healpix supports the "unique" scheme, cdshealpix-python does not expose it (cdshealpix-rust definitely implements that, though)

jmdelouis commented 2 weeks ago

The story of the Healpix license, and then healpy, is very long. My understanding is that it will be impossible to change the healpy license. I would be very disappointed to use anything else to reach healpy users. People are usually very conservative and might be hesitant to use results from any other library.

keewis commented 2 weeks ago

I would be very disappointed to use anything else to reach healpy users

I agree, this is definitely not ideal. The the vast majority of the scientific python ecosystem uses permissive licenses so we should do the same, but if we use healpy we can't really – or rather GPL is pretty uncertain about whether importing is possible or not (it was written when interpreted languages were rare). I've spent yesterday and today trying to figure that out, and couldn't find a clear answer. To finally wrap this up I think it's easier to just side-step the issue entirely.

Note, however, that we were only ever using the functions from the healpy.pixelfunc module, which is sufficiently easy to validate (so no spherical harmonics where I would understand the need to trust the library). I noticed that cdshealpix produces slightly different cell centers for the example datasets, but these differences were around 10⁻¹³ for order 13, (which is about 0.6µm on Earth), so at least for geospatial applications I wouldn't worry about that too much. This might be different for astronomy, though.

If you truly need a healpy-backed implementation we can always create a separate library that provides that (and license it as GPLv2), we'd just have to figure out how to select the desired index implementation.

benbovy commented 2 weeks ago

It might be worth checking if the alternative implementations provide features available in healpy like basic interpolation and cell neighbors. We don't use those here yet I think but we might want to use it in the future (e.g., #8).

I haven't checked it but if those alternative implementations are based on healpix_bare it is likely that they do not provide those features.

This also raises the question of having a plug-in system for adding 3rd-party DGGS (implementations)?

keewis commented 2 weeks ago

This also raises the question of having a plug-in system for adding 3rd-party DGGS

we kind of already have this plugin system (see https://github.com/xarray-contrib/xdggs/issues/43#issuecomment-2457203413), we just have to formalize it and maybe add an entrypoint (but I don't think the latter needs to be a priority)

keewis commented 2 weeks ago

provide features available in healpy like basic interpolation and cell neighbors

The interpolation in healpy and astropy-healpix and cdshealpix (for the nested scheme) only allows interpolating from cells to arbitrary locations (but maybe that's all we need?). All three support computing immediate cell neighbours (I couldn't find docs for healpix-bare, so I can't confirm whether that also supports that).

tinaok commented 2 weeks ago

provide features available in healpy like basic interpolation and cell neighbors

The interpolation in healpy and astropy-healpix and cdshealpix (for the nested scheme) only allows interpolating from cells to arbitrary locations (but maybe that's all we need?). All three support computing immediate cell neighbours (I couldn't find docs for healpix-bare, so I can't confirm whether that also supports that).

@jmdelouis has example of code doing spline on healpix, but looked most part is written by himself.

keewis commented 1 week ago

it looks like the reason for the failing docs build is that healpy and cdshealpix slightly disagree on the cell center coordinates. Not sure how to resolve this besides regenerating the examples in xdggs-data or switching to xr.testing.assert_allclose.