Resolve version conflicts and overwritten attribute in copying (#87)

* Don't deepcopy scipy objects.

* Bump to v3.

* Compare safely in any supported scipy version.

* Skip more steps in init during copy.

* Reorder operations during copyinit.

* Reorder operations during copyinit.

* Fix bug in test.

* Add a regression test.

* Remove unused if statements, add clarifying comment.

Author: kyleaoman

codemeta.json

@@ -19,6 +19,6 @@
     "codeRepository": [
         "https://github.com/SWIFTSIM/swiftgalaxy",
     ],
-    "version": "2.5.0",
+    "version": "3.0.0",
     "license": "https://spdx.org/licenses/GPL-3.0-only.html",
 }

pyproject.toml

@@ -4,7 +4,7 @@ build-backend = "setuptools.build_meta"
 
 [project]
 name = "swiftgalaxy"
-version="2.5.0"
+version="3.0.0"
 authors = [
     { name="Kyle Oman", email="kyle.a.oman@durham.ac.uk" },
 ]

swiftgalaxy/halo_catalogues.py

@@ -1853,12 +1853,14 @@ def lazy_mask() -> Union[NDArray, slice]:
                 if not hasattr(cat, list_name):
                     return null_slice
                 mask = getattr(cat, list_name)
-                mask = mask[in_one_of_ranges(mask, getattr(sg.mask, group_name))]
+                mask = mask[
+                    in_one_of_ranges(mask, getattr(sg._spatial_mask, group_name))
+                ]
                 mask = np.isin(
                     np.concatenate(
                         [
                             np.arange(start, end)
-                            for start, end in getattr(sg.mask, group_name)
+                            for start, end in getattr(sg._spatial_mask, group_name)
                         ]
                     ),
                     mask,

swiftgalaxy/reader.py

@@ -71,12 +71,20 @@ def _apply_box_wrap(
     :class:`~swiftsimio.objects.cosmo_array`
         The coordinates wrapped to lie within the box dimensions.
     """
+    rotation_is_identity = (
+        True
+        if current_transform is None
+        else current_transform.rotation.approx_equal(Rotation.identity())
+    )
+    # in scipy 1.16 approx_equal returns bool, in 1.17 returns array of bool, so:
+    rotation_is_identity = (
+        rotation_is_identity.all()
+        if hasattr(rotation_is_identity, "all")
+        else rotation_is_identity
+    )
     if boxsize is None:
         return coords
-    elif (
-        current_transform is None
-        or (current_transform.rotation.approx_equal(Rotation.identity())).squeeze()
-    ):
+    elif current_transform is None or rotation_is_identity:
         return (coords + offset_frac * boxsize) % boxsize - offset_frac * boxsize
     else:
         return _apply_rotation(
@@ -1566,10 +1574,8 @@ def __init__(
         self.id_particle_dataset_name = id_particle_dataset_name
         self.coordinates_dataset_name = coordinates_dataset_name
         self.velocities_dataset_name = velocities_dataset_name
-        if not hasattr(self, "_coordinate_like_transform"):
-            self._coordinate_like_transform = RigidTransform.identity()
-        if not hasattr(self, "_velocity_like_transform"):
-            self._velocity_like_transform = RigidTransform.identity()
+        self._coordinate_like_transform = RigidTransform.identity()
+        self._velocity_like_transform = RigidTransform.identity()
         if self.halo_catalogue is None:
             # in server mode we don't have a halo_catalogue yet
             self._spatial_mask = getattr(self, "_spatial_mask", None)
@@ -1790,15 +1796,10 @@ def _copyinit(
         """
         sg = cls.__new__(cls)
         sg._spatial_mask = _spatial_mask
-        sg._extra_mask = _extra_mask
-        if _coordinate_like_transform is not None:
-            sg._coordinate_like_transform = _coordinate_like_transform
-        if _velocity_like_transform is not None:
-            sg._velocity_like_transform = _velocity_like_transform
         SWIFTGalaxy.__init__(
             sg,
             snapshot_filename,
-            halo_catalogue,
+            halo_catalogue=None,
             auto_recentre=auto_recentre,
             transforms_like_coordinates=transforms_like_coordinates,
             transforms_like_velocities=transforms_like_velocities,
@@ -1808,6 +1809,13 @@ def _copyinit(
             coordinate_frame_from=coordinate_frame_from,
             _data_server=_data_server,
         )
+        if _extra_mask is not None:
+            sg._extra_mask = _extra_mask
+        if _coordinate_like_transform is not None:
+            sg._coordinate_like_transform = _coordinate_like_transform
+        if _velocity_like_transform is not None:
+            sg._velocity_like_transform = _velocity_like_transform
+        sg.halo_catalogue = halo_catalogue
         return sg
 
     def __str__(self) -> str:
@@ -1930,8 +1938,8 @@ def _data_copy(
             velocities_dataset_name=deepcopy(self.velocities_dataset_name),
             _spatial_mask=self._spatial_mask,
             _extra_mask=deepcopy(self._extra_mask),
-            _coordinate_like_transform=deepcopy(self._coordinate_like_transform),
-            _velocity_like_transform=deepcopy(self._velocity_like_transform),
+            _coordinate_like_transform=copy(self._coordinate_like_transform),
+            _velocity_like_transform=copy(self._velocity_like_transform),
             _data_server=_data_server,
         )
         for particle_name in sg.metadata.present_group_names:

tests/test_masking.py

@@ -3,7 +3,9 @@
 import pytest
 from copy import copy, deepcopy
 import numpy as np
+import unyt as u
 from unyt.testing import assert_allclose_units
+from swiftsimio import cosmo_quantity
 from swiftgalaxy import SWIFTGalaxy, MaskCollection
 from swiftgalaxy.demo_data import (
     ToyHF,
@@ -188,6 +190,38 @@ def test_bool_mask(self, sg, particle_name, before_load):
         ids = masked_dataset.particle_ids
         assert_allclose_units(ids_before[mask], ids, rtol=0, atol=0)
 
+    def test_chaining_masks(self, sg):
+        """
+        Check that we can mask a particle dataset after masking the swiftgalaxy.
+
+        This is a regression test, but with no associated github issue.
+        """
+        sg.mask_particles(
+            MaskCollection(
+                gas=sg.gas.spherical_coordinates.r
+                < cosmo_quantity(
+                    3,
+                    u.kpc,
+                    comoving=True,
+                    scale_factor=sg.metadata.scale_factor,
+                    scale_exponent=1,
+                )
+            )
+        )
+        # this had previously caused a crash in version <=2.4.1:
+        # IndexError: boolean index did not match indexed array along axis 0;
+        # size of axis is 5000 but size of corresponding boolean axis is 1480
+        sg.gas[
+            sg.gas.spherical_coordinates.r
+            > cosmo_quantity(
+                1,
+                u.kpc,
+                comoving=True,
+                scale_factor=sg.metadata.scale_factor,
+                scale_exponent=1,
+            )
+        ]
+
 
 class TestMaskingNamedColumnDatasets:
     """Test applying masks to named column datasets."""
@@ -219,7 +253,7 @@ def test_reordering_int_mask(self, sg, before_load):
         if before_load:
             sg.gas.hydrogen_ionization_fractions._neutral = None
             del sg._extra_mask.gas._mask
-            sg._extra_mask.gas._mask = False
+            sg._extra_mask.gas._evaluated = False
         masked_namedcolumnsdataset = sg.gas.hydrogen_ionization_fractions[mask]
         fractions = masked_namedcolumnsdataset.neutral
         assert_allclose_units(