Merge pull request #149 from cds-astro/fix-bad-negative-pixels

Filter out negative indices in `from_healpix_cells` and in `from_valued_healpix_cells`

CHANGELOG.md

@@ -25,11 +25,18 @@ between the small MOC described by the polygon or the bigger one (its complement
 
 ### Changed
 
+* `MOC.from_healpix_cells` also accepts an int as depth if all the cells are at the same level
 * `MOC.from_vizier_table()` does not call the MOCServer anymore. It now raises an error if the
 catalog of table name is invalid (see #143). It also accepts `max_depth` as an argument. This
 should replace `nside` in a future version.
 * `MOC.from_ivorn()` now accepts `max_depth` as an argument. This should reb=place `nside` later.
 
+### Fixed
+
+* `ranges` in `from_depth29_ranges` is now optional, to be consistent with the existing docstring
+* `from_healpix_cells` and `from_velued_healpix_cells` now filter out invalid cells and raise a
+warning when they do so
+
 ## [0.15.0]
 
 ### Added

python/mocpy/moc/moc.py

@@ -1,5 +1,7 @@
 import contextlib
 import functools
+import warnings
+from collections.abc import Iterable
 from io import BytesIO
 from math import log2
 from pathlib import Path
@@ -79,6 +81,28 @@ def _validate_lonlat_wrap(self, lon, lat, **kwargs):
     return _validate_lonlat_wrap
 
 
+def _mask_unsigned_before_casting(indices):
+    """Return a mask for an array of integers if there are negative values.
+
+    This is useful before casting indices into unsigned integers.
+
+    Parameters
+    ----------
+    indices : `numpy.ndarray` or Iterable
+    """
+    if np.issubdtype(np.asarray(indices).dtype, np.unsignedinteger) or all(
+        np.asarray(indices) > 0,
+    ):
+        return None
+    warnings.warn(
+        "The list of indices contain negative values. They are filtered "
+        "out to generate the MOC",
+        UserWarning,
+        stacklevel=2,
+    )
+    return np.array(indices) > 0
+
+
 class MOC(AbstractMOC):
     """
     Multi-order spatial coverage class.
@@ -131,8 +155,7 @@ def __init__(self, store_index):
     @property
     def max_order(self):
         """Depth/order of the S-MOC."""
-        depth = mocpy.get_smoc_depth(self.store_index)
-        return np.uint8(depth)
+        return mocpy.get_smoc_depth(self.store_index)
 
     @classmethod
     def n_cells(cls, depth):
@@ -264,8 +287,6 @@ def contains(self, lon, lat, keep_inside=True):
         --------
         contains_skycoords
         """
-        import warnings
-
         warnings.warn(
             "This method is deprecated and has been replaced by contains_lonlat",
             DeprecationWarning,
@@ -429,8 +450,6 @@ def get_boundaries(self, order=None):
         coords: [`~astropy.coordinates.SkyCoord`]
             A list of `~astropy.coordinates.SkyCoord` each describing one border.
         """
-        import warnings
-
         warnings.warn(
             "This method is not stable. A future more stable algorithm will be implemented!",
             DeprecationWarning,
@@ -582,8 +601,6 @@ def from_vizier_table(cls, table_id, max_depth=None, nside=None):
         currently available.
         """
         if nside:
-            import warnings
-
             warnings.warn(
                 "'nside' is deprecated in favor of 'max_depth'. We use the nside"
                 "value for this request. You can switch to max_depth with "
@@ -652,8 +669,6 @@ def from_ivorn(cls, ivorn, max_depth: int = 8, nside=None):
         complete implementation, see the MOCServer module in the astroquery library.
         """
         if nside:
-            import warnings
-
             warnings.warn(
                 "'nside' is deprecated in favor of 'max_depth'. We use the nside"
                 "value for this request. You can switch to max_depth with "
@@ -674,8 +689,6 @@ def from_ivorn(cls, ivorn, max_depth: int = 8, nside=None):
         )
 
         if moc.empty():
-            import warnings
-
             warnings.warn(
                 "This MOC is empty. Possible causes are that this IVORN has no "
                 "positions or this is not a valid IVORN.",
@@ -1045,8 +1058,6 @@ def from_valued_healpix_cells(
             The resulting MOC
         """
         if max_depth is None:
-            import warnings
-
             warnings.warn(
                 "To avoid an extra loop, it is preferable to provide the max_depth parameter."
                 "It will probably become mandatory in future releases.",
@@ -1060,6 +1071,12 @@ def from_valued_healpix_cells(
                     "Invalid uniq numbers. Too big uniq or negative uniq numbers might be the cause.",
                 )
 
+        mask = _mask_unsigned_before_casting(uniq)
+        # if any of the values in uniq are negative
+        if mask is not None:
+            uniq = np.array(uniq)[mask]
+            values = np.array(values)[mask]
+
         index = mocpy.from_valued_hpx_cells(
             np.uint8(max_depth),
             uniq.astype(np.uint64),
@@ -1766,7 +1783,7 @@ def from_healpix_cells(cls, ipix, depth, max_depth):
         ----------
         ipix : `numpy.ndarray`
             HEALPix cell indices in the NESTED notation. dtype must be np.uint64
-        depth : `numpy.ndarray`
+        depth : `numpy.ndarray` or int
             Depth of the HEALPix cells. Must be of the same size of `ipix`.
             dtype must be np.uint8. Corresponds to the `level` of an HEALPix cell in astropy.healpix.
         max_depth : int, The resolution of the MOC (degrades on the fly input cells if necessary)
@@ -1778,18 +1795,26 @@ def from_healpix_cells(cls, ipix, depth, max_depth):
 
         Returns
         -------
-        moc : `~mocpy.moc.MOC`
+        `~mocpy.moc.MOC`
             The MOC
         """
+        if not isinstance(depth, Iterable):
+            depth = np.full(len(ipix), depth, dtype=np.uint8)
+
+        mask = _mask_unsigned_before_casting(ipix)
+        if mask is not None:
+            ipix = np.array(ipix)[mask]
+            depth = np.array(depth)[mask]
+
         index = mocpy.from_healpix_cells(
             np.uint8(max_depth),
-            depth.astype(np.uint8),
-            ipix.astype(np.uint64),
+            np.array(depth, dtype=np.uint8),
+            np.array(ipix, dtype=np.uint64),
         )
         return cls(index)
 
     @classmethod
-    def from_depth29_ranges(cls, max_depth, ranges):
+    def from_depth29_ranges(cls, max_depth, ranges=None):
         """
         Create a MOC from a set of ranges of HEALPix Nested indices at order 29.
 
@@ -2035,8 +2060,6 @@ def plot(self, title="MOC", frame=None):
         frame : `astropy.coordinates.BaseCoordinateFrame`, optional
             Describes the coordinate system the plot will be (ICRS, Galactic are the only coordinate systems supported).
         """
-        import warnings
-
         warnings.warn(
             "This method is deprecated and is no longer tested."
             "Please refer to `MOC.fill` and `MOC.border` methods",

python/mocpy/tests/test_moc.py

@@ -230,24 +230,34 @@ def gen_f(size):
     return gen_f
 
 
-@pytest.mark.parametrize("size", [1000, 10000, 50000])
-def test_moc_from_skycoords(skycoords_gen_f, size):
-    skycoords = skycoords_gen_f(size)
-    MOC.from_skycoords(skycoords, max_norder=7)
+def test_moc_from_skycoords(skycoords_gen_f):
+    skycoords = skycoords_gen_f(10)
+    moc = MOC.from_skycoords(skycoords, max_norder=7)
+    assert all(moc.contains_skycoords(skycoords))
 
 
-@pytest.mark.parametrize("size", [1000, 10000, 50000])
-def test_moc_from_lonlat(lonlat_gen_f, size):
-    lon, lat = lonlat_gen_f(size)
-    MOC.from_lonlat(lon=lon, lat=lat, max_norder=6)
+def test_moc_from_lonlat(lonlat_gen_f):
+    lon, lat = lonlat_gen_f(10)
+    moc_lon_lat = MOC.from_lonlat(lon=lon, lat=lat, max_norder=6)
+    moc_skycoo = MOC.from_skycoords(SkyCoord(lon, lat), max_norder=6)
+    assert moc_lon_lat == moc_skycoo
 
 
 def test_from_healpix_cells():
     ipix = np.array([40, 87, 65])
     depth = np.array([3, 3, 3])
-    np.array([True, True, True])
-
-    MOC.from_healpix_cells(max_depth=29, ipix=ipix, depth=depth)
+    moc = MOC.from_healpix_cells(max_depth=3, ipix=ipix, depth=depth)
+    # the MOC is correct
+    assert moc == MOC.from_str("3/40 87 65")
+    # we can also give depth as an integer if the cells have only one order
+    assert moc == MOC.from_healpix_cells(ipix=ipix, depth=3, max_depth=3)
+    # negative indices are ignored
+    with pytest.warns(
+        UserWarning,
+        match="The list of indices contain negative values*",
+    ):
+        moc = MOC.from_healpix_cells(ipix=[40, -1, 65], depth=depth, max_depth=3)
+    assert moc == MOC.from_str("3/40 65")
 
 
 def test_from_polygons():
@@ -822,30 +832,50 @@ def test_spatial_res_to_order():
     assert (order == output).all()
 
 
-def test_from_valued_healpix_cells_empty():
-    uniq = np.array([])
-    values = np.array([])
-
-    MOC.from_valued_healpix_cells(uniq, values, 12)
-
+def test_from_valued_healpix_cells():
+    uniq = np.array([8, 12])
+    values = np.array([0.4, 0.6])
+    moc_1 = MOC.from_valued_healpix_cells(
+        uniq,
+        values,
+        cumul_from=0,
+        cumul_to=1,
+        max_depth=12,
+    )
+    assert moc_1 == MOC.from_string("0/4 8\n12/")
+    moc_0p7 = MOC.from_valued_healpix_cells(
+        uniq,
+        values,
+        cumul_from=0,
+        cumul_to=0.7,
+        max_depth=12,
+    )
+    assert moc_0p7 == MOC.from_string("0/8\n12/")
 
-def test_from_valued_healpix_cells_different_sizes():
+    # different sizes
     uniq = np.array([500])
     values = np.array([])
-
     with pytest.raises(
         ValueError,
         match="`uniq` and values do not have the same size.",
     ):
         MOC.from_valued_healpix_cells(uniq, values, 12)
-
-
-def test_from_valued_healpix_cells_cumul_from_sup_cumul_to():
-    uniq = np.array([500])
+    # failed comparison between cumul_from and cumul_to
     values = np.array([1.0])
-
     with pytest.raises(ValueError, match="`cumul_from` has to be < to `cumul_to`."):
         MOC.from_valued_healpix_cells(uniq, values, 12, cumul_from=0.8, cumul_to=-5.0)
+    # negative uniq
+    uniq = [-2, 270]
+    values = [0.2, 0.2]
+    with pytest.warns(
+        UserWarning,
+        match="The list of indices contain negative values*",
+    ):
+        assert MOC.from_valued_healpix_cells(
+            uniq,
+            values,
+            max_depth=3,
+        ) == MOC.from_string("3/14")
 
 
 @pytest.mark.parametrize(