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

Consider alternative healpix libraries #53

Open
delucchi-cmu opened this issue Mar 16, 2023 · 12 comments
Open

Consider alternative healpix libraries #53

delucchi-cmu opened this issue Mar 16, 2023 · 12 comments
Labels
help wanted Extra attention is needed

Comments

@delucchi-cmu
Copy link
Contributor

We're currently using healpy, which is a somewhat bloated library (also contains matplotlib, cfitsio, etc), and has installation issues on macOS, and is incompatible with Windows.

Alternatives suggested so far:

If any of these are a little behind healpy in terms of raw performance, but otherwise a good replacement, it may be worthwhile to contribute improvements to their code bases.

@hombit
Copy link
Contributor

hombit commented Mar 16, 2023

Performance comparison for healpy, hpgeom, cdshealpix and astropy-healpix on my laptop:

  • healpy 28.4 ms ± 78.7 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)
  • hpgeom 37.1 ms ± 377 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)
  • cdshealpix single thread 26.1 ms ± 202 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)
  • cdshealpix 8 threads 12.8 ms ± 75 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)
  • astropy-healpix 132 ms ± 261 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)
import healpy
import hpgeom
import cdshealpix
import astropy_healpix

import astropy
import numpy as np


n = 1_000_000

rng = np.random.default_rng(0)
ra = rng.uniform(0, 360, n)
dec = np.degrees(np.arccos(rng.uniform(-1, 1, n))) - 90

%timeit healpy.ang2pix(1 << 29, ra, dec, lonlat=True, nest=True)
%timeit hpgeom.angle_to_pixel(1 << 29, ra, dec, lonlat=True, degrees=True, nest=True)
%timeit cdshealpix.nested.lonlat_to_healpix(astropy.coordinates.Longitude(ra, 'deg'), astropy.coordinates.Latitude(dec, 'deg'), 29, num_threads=1)
%timeit cdshealpix.nested.lonlat_to_healpix(astropy.coordinates.Longitude(ra, 'deg'), astropy.coordinates.Latitude(dec, 'deg'), 29, num_threads=8)
%timeit astropy_healpix.lonlat_to_healpix(ra * astropy.units.deg, dec * astropy.units.deg, nside=1 << 29, order='nested')

@delucchi-cmu
Copy link
Contributor Author

Keeping this for posterity. If we can get a Windows-compatible healpix library, we can enable a windows-file-system checking regression test workflow:

# This workflow will install Python dependencies and run tests in a Windows environment.
# This is intended to catch any file-system specific issues, and so runs less
# frequently than other test suites.

name: Windows unit test

on:
  push:
    branches: [ main ]
  pull_request:
    branches: [ main ]

jobs:
  build:

    runs-on: windows-latest
    strategy:
      matrix:
        python-version: ['3.10']

    steps:
    - uses: actions/checkout@v3
    - name: Set up Python ${{ matrix.python-version }}
      uses: actions/setup-python@v4
      with:
        python-version: ${{ matrix.python-version }}
    - name: Install dependencies
      run: |
        python -m pip install --upgrade pip
        pip install .
        pip install .[dev]
    - name: Run unit tests with pytest
      run: |
        python -m pytest tests

@delucchi-cmu
Copy link
Contributor Author

Ugh. Another ding.

#65

@delucchi-cmu delucchi-cmu added the help wanted Extra attention is needed label Mar 28, 2023
@maxwest-uw
Copy link
Contributor

going through all the available healpix implementations and checking to see if they've ported all the healpy functions that we need to handle margin caching.

hpgeom

  • has everything!

cdshealpix

  • no boundaries function 😭
  • however they do have external_neighbors, which is essentially our get_margin code but implemented in rust 👀

astropy-healpix

  • has everything!
  • has boundaries_lonlat which does the conversion of boundary coords into ra/dec, which healpy annoyingly doesn't do by default so that's kinda nice.

was really rooting for cdshealpix but it unfortunately doesn't have all the necessary functions ported over. if they do end up implementing healpy.boundaries we should consider using it as it's fast and could even up allowing us to delete some code.

@maxwest-uw
Copy link
Contributor

small update on this, since the margin cache code has been significantly changed: we're still reliant on the healpy.boundaries function, probably more so now in fact 😭

@eteq
Copy link

eteq commented Feb 16, 2024

Coming back here after some out-of-band discussion at a hipscat WG meeting: am I reading it right that astropy-healpix is compatible with everything, but the main restriction is performance? (basing that on #53 (comment)) And if so, any insight on where the performance hotspot is @delucchi-cmu ?

@eteq
Copy link

eteq commented Feb 16, 2024

(to be clear, I also am interested in cdshealpix given the rust under-layer, since it seems to support a lot of the same user-level constructs that I like about astropy-healpix, but if I am reading the above it seems like it's out of the running without boundaries?)

@hombit
Copy link
Contributor

hombit commented Feb 23, 2024

@eteq @delucchi-cmu I slightly updated the original benchmarking comment #53 (comment), basically fixing typos in the code. The performance of the packages hasn't change since I tested it last time.

@DaftPict
Copy link

I just happened to need cdshealpix for another project and in reading the docs noticed that another package it can use (mocpy) had a get_boundaries function. As a old-school astronomer rather than a modern developer I just wondered if this would solve the missing boundaries issue mentioned earlier and allow a switch to cdshealpix? I'm stuck on windows and would love a version I can use locally.

@fxpineau
Copy link

Hi,
I am the developer of cds-healpix-rust, @tboch talked to me about this issue.
I think that the equivalent of boundaries in cdshealxpix is path_along_cell_edge.
If needed, we can add it in the cdshealpix python wrapper (cc @bmatthieu3, @ManonMarchand).

In such cases (feature requests), do not hesitate to open issues.

@ManonMarchand
Copy link

Yes, don't hesitate to interact either on mocpy or cdshealpix repos. You're more than welcome to ask for features, point places in the docs where we're not clear, point to bugs or slow methods... or even contribute 🙂
If github issues are not your preferred communication canal, our emails are also in the documentation

@fxpineau
Copy link

fxpineau commented Jun 20, 2024

I just checked the code.
Using vertices_skycoord with step>1 corresponds to the boundaries method: when step is >1 the Rust method path_along_cell_edge is used internally. See this line of code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
Status: No status
Development

No branches or pull requests

7 participants