cds-astro / mocpy

Python library to easily create and manipulate MOCs (Multi-Order Coverage maps)
https://cds-astro.github.io/mocpy/
BSD 3-Clause "New" or "Revised" License
60 stars 33 forks source link

moc.union(moc1, moc2) fails with TypeError #153

Closed hombit closed 4 months ago

hombit commented 4 months ago

While moc.union(moc1) works well, moc.union(moc1, moc2, ...) fails with a TypeError:

import astropy.units as u
from astropy.coordinates import Latitude, Longitude, Angle
from mocpy import MOC

moc = MOC.from_cone(lon=Longitude(0, u.deg), lat=Latitude(0, u.deg), radius=Angle(1, u.arcmin), max_depth=19)
moc.union(moc, moc)
File ~/.virtualenvs/lsdb/lib/python3.11/site-packages/mocpy/abstract_moc.py:323, in AbstractMOC.union(self, another_moc, *args)
    315 if args:
    316     store_indices = np.append(
    317         [self.store_index, another_moc.store_index],
    318         np.fromiter(
   (...)
    321         ),
    322     )
--> 323     index = mocpy.multi_union(store_indices)
    324 else:
    325     index = mocpy.union(self.store_index, another_moc.store_index)

TypeError: argument 'ids': type mismatch:
 from=float64, to=uint64

Tested with mocpy v0.15.0, astropy v6.0.0, numpy v1.24.4, python 3.11, macOS 14.5

ManonMarchand commented 4 months ago

Indeed! Thanks for reporting.

While we do the fix, you can use sum that also calls union behind the scenes:

import astropy.units as u
from astropy.coordinates import Latitude, Longitude, Angle
from mocpy import MOC

moc = MOC.from_cone(lon=Longitude(0, u.deg), lat=Latitude(0, u.deg), radius=Angle(1, u.arcmin), max_depth=19)
sum([moc, moc, moc]) == moc
True
ManonMarchand commented 4 months ago

It's fixed in master if you can work from there. Otherwise, the next release should be in a few days. Thanks again for reporting the issue :slightly_smiling_face:

hombit commented 4 months ago

Thank you! .union works so much faster than sum

ManonMarchand commented 4 months ago

Yes, sum does the operation with python while union does it on the rust side (except for STMOCs for now) @fxpineau will be happy to read that the difference is noticeable :wink:

fxpineau commented 4 months ago

FYI, the algorithm is not the same. But, if I am correct, both are made on the Rust side. sum makes successive unions while a kway-merge (with k=4) is performed in the case of union with more than 2 parameters.