Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

replace healpy with cdshealpix #92

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

keewis
Copy link
Collaborator

@keewis keewis commented Nov 14, 2024

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

@keewis keewis requested a review from benbovy November 14, 2024 14:15
@tinaok
Copy link
Contributor

tinaok commented Nov 14, 2024

@zawadzl

@benbovy
Copy link
Member

benbovy commented Nov 14, 2024

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
Copy link
Collaborator Author

keewis commented Nov 14, 2024

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
Copy link
Member

benbovy commented Nov 14, 2024

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
Copy link
Collaborator Author

keewis commented Nov 14, 2024

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
Copy link

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
Copy link
Collaborator Author

keewis commented Nov 14, 2024

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
Copy link
Member

benbovy commented Nov 14, 2024

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
Copy link
Collaborator Author

keewis commented Nov 14, 2024

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 #43 (comment)), we just have to formalize it and maybe add an entrypoint (but I don't think the latter needs to be a priority)

@keewis
Copy link
Collaborator Author

keewis commented Nov 14, 2024

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
Copy link
Contributor

tinaok commented Nov 14, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants