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

Making pyssht and healpy optional dependencies #224

Closed
matt-graham opened this issue Sep 26, 2024 · 1 comment
Closed

Making pyssht and healpy optional dependencies #224

matt-graham opened this issue Sep 26, 2024 · 1 comment
Labels
enhancement New feature or request wontfix This will not be worked on

Comments

@matt-graham
Copy link
Collaborator

My understanding is the only place in the package code (as opposed to tests and notebooks) that healpy and pyssht are imported is

import pyssht
import healpy

with this module providing JAX wrappers around the transforms in these packages.

Installing healpy and/or pyssht can be problematic on some operating systems and platforms - for example there is currently no pre-built Windows wheels for either (and building package from source on Windows appears to fail from some attemps in a CI run in #210, and we have also had problems getting healpy installed on a MacOS12 / arm64 machine due to lack of a pre-built wheel being available and errors linking against dynamic libraries when using a package built from source.

To make s2fft installable on a wider range of platforms, it might therefore be a good idea to drop pyssht and healpy as required dependencies as instead have them as optional dependencies with the functions requiring them in s2fft/transforms/c_backend_spherical.py only made available when the relevant modules are installed.

@matt-graham matt-graham added the enhancement New feature or request label Sep 26, 2024
@jasonmcewen jasonmcewen added the wontfix This will not be worked on label Sep 27, 2024
@jasonmcewen
Copy link
Contributor

I appreciate we've been having some installation issues with pyssht and healpy lately. Although in general they shouldn't be problematic. These codes should indeed only be used for the C backends and also for regression testing.

I see there is an argument for making them optional, although I'm not 100% sure about that. If the problematic installation is no longer an issue, then there is also an argument for keeping these. It really depends on how widely they are used and whether we want to put more installation effort on those users that are interested in this support.

I appreciate including these codes as dependencies restricts our Windows suport. However, we're not targetting Windows at present (just as these other codes don't). My guess is that we'd have more users interested in this functionality than total number of Windows users.

I suggest we continue to monitor the situation but don't plan to action this at present.

@matt-graham @CosmoMatt I'll close this issue for now and tag as wontfix but we can reopen it in future if we think we should action this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

2 participants