-
-
Notifications
You must be signed in to change notification settings - Fork 23
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
cdshealpix, a Rust HEALPix implementation #128
Comments
@cdeil - @fxpineau and I have implemented the basic methods for the ring scheme. Please see the doc of the API for all the methods supported: The package has two sub packages nested and ring that each have their own methods: from cdshealpix.nested import lonlat_to_healpix
from cdshealpix.ring import lonlat_to_healpix The nested schema methods take a There are two new methods in cdshealpix, Finally there is also a new |
@bmatthieu3 @fxpineau @tboch - Thank you for all your work on the CDS HEALPix codes! As I mentioned at PyGamma19, I would consider a C HEALPix lib that we bundle and wrap for For now, I agree that changing @astrofrog @lpsinger - OK to go this way? @bmatthieu3 - How do you propose we set this up? Would we depend on and import I guess the second option is more maintainable and better for users (no "should I use One thing we can do for now is to improve tests a bit further, e.g. add CI builds for the packages that use |
My main concern is that the core routines are still Numpy ufuncs so that they get broadcasted over indices and types appropriately. I am not familiar with Rust build tools so I don't know how to call Rust functions from a Python C extension. |
@lpsinger - The way I do it is in pure Rust because:
There is a rust wrapper library around numpy called rust-numpy that handles This is an example of #[pyfn(m, "lonlat_to_healpix")]
fn lonlat_to_healpix(_py: Python,
depth: u8,
lon: &PyArrayDyn<f64>,
lat: &PyArrayDyn<f64>,
ipix: &PyArrayDyn<u64>,
dx: &PyArrayDyn<f64>,
dy: &PyArrayDyn<f64>)
-> PyResult<()> {
let lon = lon.as_array();
let lat = lat.as_array();
let mut ipix = ipix.as_array_mut();
let mut dx = dx.as_array_mut();
let mut dy = dy.as_array_mut();
let layer = healpix::nested::get_or_create(depth);
Zip::from(&mut ipix)
.and(&mut dx)
.and(&mut dy)
.and(&lon)
.and(&lat)
.par_apply(|p, x, y, &lon, &lat| {
let r = layer.hash_with_dxdy(lon, lat);
*p = r.0;
*x = r.1;
*y = r.2;
});
Ok(())
} The This code is then compiled into a .so shared library using def lonlat_to_healpix(lon, lat, depth, return_offsets=False):
lon = np.atleast_1d(lon.to_value(u.rad))
lat = np.atleast_1d(lat.to_value(u.rad))
if depth < 0 or depth > 29:
raise ValueError("Depth must be in the [0, 29] closed range")
if lon.shape != lat.shape:
raise ValueError("The number of longitudes does not match with the number of latitudes given")
num_ipix = lon.shape
# Allocation of the array containing the resulting HEALPix cell indices
# This is done in the Python side code to benefit the Python garbage collector
ipix = np.empty(num_ipix, dtype=np.uint64)
# And the offsets
dx = np.empty(num_ipix, dtype=np.float64)
dy = np.empty(num_ipix, dtype=np.float64)
# Call the lonlat_to_healpix from the shared library (.so)
cdshealpix.lonlat_to_healpix(depth, lon, lat, ipix, dx, dy)
if return_offsets:
return ipix, dx, dy
else:
return ipix |
@bmatthieu3 - Interesting. I see that you don't use
So Rust / Rayon really has a way to ship multi-threading binaries that are completely self-contained? Also on Windows? I think still we need to have a conversation whether to use multi-threading or single-threading. I just tried
It was fluctuating one to four cores, not running consistently on four cores. Overall I would probably argue again that we just don't use multi-threading to keep it simple. But that's a detail - the big question is whether to adopt |
IMO packaging and distribution maintenance effort for the coming years is the main concern with introducing Rust. There's also the fact that pretty much no-one knows Rust yet or is using it in the Astropy community, people know C or C++ or Cython or Julia, but after looking at Rust a bit I think it's fine if you and CDS supports it -- it's complex, but C Numpy Ufunc code is also complex and Cython can be tricky, it's all about the same and it is maintainable, we don't need many people to hack on that part of the code. So concerning distribution: did you try making a conda package via conda-forge? Do they have any packages using Rust or it's possible to do it? Maybe you could ask on the conda-forge gitter and try? Currently I think for Astropy and most packages users use Concerning the wheel build, it would also be good to not have a special setup that you maintain, but have it as part of a larger build setup. @astrofrog recently started https://github.com/astropy/wheel-forge - I don't know how much work it would be to build cdshealpix there. I know doing the build and deployment setup is a lot of work - I'm not suggesting you change now when it's not clear which way to go, but long-term we need a low-effort to maintain solution. |
I tried to build
@bmatthieu3 - Will Rust nightly be required for the forseeable future or will it be possible to compile |
Ok! I will work on that and will keep you in touch. For the wheel building @astrofrog already said to me about https://github.com/astropy/wheel-forge. I fully agree with you, a special setup as it is for now is not a good long-term solution. @cdeil - Only test and benchmark features need the nightly version of the rust compiler so that is not core code and thus this should be removed soon. For the moment, you can switch the compiler to the nightly with this command: rustup default nightly and go back to the stable with: rustup default stable |
@bmatthieu3 - Now I get this compile error with |
@bmatthieu3, my concern is that the Rust implementation does not broadcast over indices because it is not providing a Numpy ufunc. For example, it chokes on this: >>> import numpy as np
>>> import astropy_healpix as ah
>>> import cdshealpix as ch
>>> ipix = np.arange(3)
>>> level = np.arange(2)
>>> nside = 2**level
>>> ah.healpix_to_lonlat(ipix[:, np.newaxis], nside[np.newaxis, :])
(<Longitude [[0.78539816, 0.78539816],
[2.35619449, 2.35619449],
[3.92699082, 3.92699082]] rad>, <Latitude [[0.72972766, 1.15965846],
[0.72972766, 1.15965846],
[0.72972766, 1.15965846]] rad>)
>>> ch.healpix_to_lonlat(ipix[:, np.newaxis], level[np.newaxis, :])
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/usr/lib/python3.7/site-packages/cdshealpix/nested/healpix.py", line 155, in healpix_to_lonlat
if depth < 0 or depth > 29:
ValueError: The truth value of an array with more than one element is ambiguous. Use a.any() or a.all() It's important for our LIGO applications that these functions broadcast correctly. |
cdshealpix is now in conda-forge and does not require nightly builds any more:
|
Interestingly, the popular |
@cdeil @fxpineau @astrofrog @tboch - Following our discussion from PyGamma19, a mid-term goal is to replace the current C HEALPix implementation from astrometry.net with the new Rust one (github link) developed by @fxpineau. The assumption behind that being that astropy_healpix will remain an affiliated package (because Rust is currently too recent to be moved to the core of astropy).
On my side, I continued my developments on cdshealpix (github link) and released a version 0.2.0 where:
cdshealpix has some features that astropy_healpix does not have:
And some features from astropy_healpix are missing in cdshealpix. Those are:
The text was updated successfully, but these errors were encountered: