From 7f80a7d91244bc5ccc1a505af081139fb2e2223d Mon Sep 17 00:00:00 2001 From: Nabil Freij Date: Tue, 20 Oct 2026 12:06:44 -0700 Subject: [PATCH 1/8] Stop NDCube._new_instance copying the whole cube through back-references --- changelog/+new-instance-copies.bugfix.rst | 1 + ndcube/ndcube.py | 10 ++++--- ndcube/tests/test_ndcube_arithmetic.py | 33 +++++++++++++++++++++++ 3 files changed, 41 insertions(+), 3 deletions(-) create mode 100644 changelog/+new-instance-copies.bugfix.rst diff --git a/changelog/+new-instance-copies.bugfix.rst b/changelog/+new-instance-copies.bugfix.rst new file mode 100644 index 000000000..dcd076d5c --- /dev/null +++ b/changelog/+new-instance-copies.bugfix.rst @@ -0,0 +1 @@ +Arithmetic on an `~ndcube.NDCube` and `ndcube.NDCube.to` no longer make a temporary copy of the whole cube, data included, when copying the uncertainty, extra coordinates and global coordinates for the result, and no longer copy the attributes the operation replaces. diff --git a/ndcube/ndcube.py b/ndcube/ndcube.py index d712f79a4..5e470d998 100644 --- a/ndcube/ndcube.py +++ b/ndcube/ndcube.py @@ -954,8 +954,11 @@ def plot(self, *args, **kwargs): return self.plotter.plot(*args, **kwargs) def _new_instance(self, **kwargs): + # The uncertainty, extra coords and global coords refer back to this cube. + # Map it in the memo so deepcopy does not copy the whole cube through them. + memo = {id(self): None} keys = ('unit', 'wcs', 'mask', 'meta', 'uncertainty', 'psf') - new_kwargs = {k: deepcopy(getattr(self, k, None)) for k in keys} + new_kwargs = {k: deepcopy(getattr(self, k, None), memo) for k in keys if k not in kwargs} # To support old versions of astropy, we need to make sure # we only pass in the parameters that are valid for the NDData params = list(inspect.signature(astropy.nddata.NDData).parameters) @@ -964,10 +967,11 @@ def _new_instance(self, **kwargs): full_kwargs['data'] = self.data full_kwargs.update(kwargs) new_cube = type(self)(**full_kwargs) + memo[id(self)] = new_cube if self.extra_coords is not None: - new_cube._extra_coords = deepcopy(self.extra_coords) + new_cube._extra_coords = deepcopy(self.extra_coords, memo) if self.global_coords is not None: - new_cube._global_coords = deepcopy(self.global_coords) + new_cube._global_coords = deepcopy(self.global_coords, memo) return new_cube def __neg__(self): diff --git a/ndcube/tests/test_ndcube_arithmetic.py b/ndcube/tests/test_ndcube_arithmetic.py index 29b0275b6..228a3ac6e 100644 --- a/ndcube/tests/test_ndcube_arithmetic.py +++ b/ndcube/tests/test_ndcube_arithmetic.py @@ -1,3 +1,6 @@ +import operator +import tracemalloc + import numpy as np import pytest @@ -431,6 +434,36 @@ def test_cube_arithmetic_neg(ndcube_2d_ln_lt_units): ) +@pytest.mark.parametrize("operation", [operator.neg, lambda cube: cube * 2], ids=["neg", "mul"]) +def test_cube_arithmetic_does_not_copy_cube(operation): + shape = (2500, 2500) # 50 MB of float64 + cube = NDCube(np.ones(shape), wcs=astropy.wcs.WCS(naxis=2), unit=u.ct, + uncertainty=StdDevUncertainty(np.ones(shape)), meta={"a": 1}) + cube.extra_coords.add("time", 0, np.arange(shape[0]) * u.s) + cube.global_coords.add("name", "custom:physical_type", 1 * u.s) + + tracemalloc.start() + try: + new_cube = operation(cube) + peak = tracemalloc.get_traced_memory()[1] + finally: + tracemalloc.stop() + # The new data and uncertainty take up to 150 MB. Copying the uncertainty or + # coordinates used to copy the cube through their back-reference, 100 MB each time. + assert peak < 4 * cube.data.nbytes + assert new_cube.extra_coords._ndcube is new_cube + assert new_cube.global_coords._ndcube is new_cube + + new_cube.meta["a"] = 2 + new_cube.uncertainty.array[0, 0] = 5 + new_cube.extra_coords.add("exposure", 1, np.arange(shape[1]) * u.s) + new_cube.global_coords.add("other", "custom:physical_type", 2 * u.s) + assert cube.meta["a"] == 1 + assert cube.uncertainty.array[0, 0] == 1 + assert list(cube.extra_coords.keys()) == ["time"] + assert list(cube.global_coords) == ["name"] + + def test_add_unitless_cube_typeerror(ndcube_2d_ln_lt_units): with pytest.raises(TypeError): _ = ndcube_2d_ln_lt_units + 10.0 From 8fa5440b8a2cf7593935f0a731d045f8b36f54e8 Mon Sep 17 00:00:00 2001 From: Nabil Freij Date: Tue, 20 Oct 2026 12:19:17 -0700 Subject: [PATCH 2/8] Bound each operation's memory peak in the new-instance copy test --- ndcube/tests/test_ndcube_arithmetic.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/ndcube/tests/test_ndcube_arithmetic.py b/ndcube/tests/test_ndcube_arithmetic.py index 228a3ac6e..2805431a7 100644 --- a/ndcube/tests/test_ndcube_arithmetic.py +++ b/ndcube/tests/test_ndcube_arithmetic.py @@ -434,8 +434,9 @@ def test_cube_arithmetic_neg(ndcube_2d_ln_lt_units): ) -@pytest.mark.parametrize("operation", [operator.neg, lambda cube: cube * 2], ids=["neg", "mul"]) -def test_cube_arithmetic_does_not_copy_cube(operation): +@pytest.mark.parametrize(("operation", "limit"), [(operator.neg, 2.5), (lambda cube: cube * 2, 3.5)], + ids=["neg", "mul"]) +def test_cube_arithmetic_does_not_copy_cube(operation, limit): shape = (2500, 2500) # 50 MB of float64 cube = NDCube(np.ones(shape), wcs=astropy.wcs.WCS(naxis=2), unit=u.ct, uncertainty=StdDevUncertainty(np.ones(shape)), meta={"a": 1}) @@ -448,9 +449,10 @@ def test_cube_arithmetic_does_not_copy_cube(operation): peak = tracemalloc.get_traced_memory()[1] finally: tracemalloc.stop() - # The new data and uncertainty take up to 150 MB. Copying the uncertainty or - # coordinates used to copy the cube through their back-reference, 100 MB each time. - assert peak < 4 * cube.data.nbytes + # The new data and uncertainty take 100 MB for neg and 150 MB for mul. + # Copying the uncertainty used to copy the cube's data through parent_nddata (50 MB). + # Copying extra_coords or global_coords used to copy the cube with its uncertainty (100 MB). + assert peak < limit * cube.data.nbytes assert new_cube.extra_coords._ndcube is new_cube assert new_cube.global_coords._ndcube is new_cube From 26356cf88ea21421cfa0bb33abc90c822f9b3ae0 Mon Sep 17 00:00:00 2001 From: Nabil Freij Date: Tue, 20 Oct 2026 12:36:29 -0700 Subject: [PATCH 3/8] Copy fill_masked's uncertainty without the cube, and multiply without copying the new uncertainty again --- changelog/+new-instance-copies.bugfix.rst | 2 +- ndcube/ndcube.py | 5 +++-- ndcube/tests/test_ndcube_arithmetic.py | 15 ++++++++------- 3 files changed, 12 insertions(+), 10 deletions(-) diff --git a/changelog/+new-instance-copies.bugfix.rst b/changelog/+new-instance-copies.bugfix.rst index dcd076d5c..b51189104 100644 --- a/changelog/+new-instance-copies.bugfix.rst +++ b/changelog/+new-instance-copies.bugfix.rst @@ -1 +1 @@ -Arithmetic on an `~ndcube.NDCube` and `ndcube.NDCube.to` no longer make a temporary copy of the whole cube, data included, when copying the uncertainty, extra coordinates and global coordinates for the result, and no longer copy the attributes the operation replaces. +Arithmetic on an `~ndcube.NDCube`, `ndcube.NDCube.to` and `ndcube.NDCube.fill_masked` no longer make a temporary copy of the whole cube, data included, when copying the uncertainty, extra coordinates and global coordinates for the result, and no longer copy the attributes the operation replaces. diff --git a/ndcube/ndcube.py b/ndcube/ndcube.py index 5e470d998..b152630c2 100644 --- a/ndcube/ndcube.py +++ b/ndcube/ndcube.py @@ -1105,7 +1105,7 @@ def __mul__(self, value): else: kwargs["unit"] = self.unit kwargs["data"] = self.data * value - kwargs["uncertainty"] = (type(self.uncertainty)(self.uncertainty.array * value) + kwargs["uncertainty"] = (type(self.uncertainty)(self.uncertainty.array * value, copy=False) if self.uncertainty is not None else None) return self._new_instance(**kwargs) @@ -1465,7 +1465,8 @@ def fill_masked(self, fill_value, uncertainty_fill_value=None, unmask=False, fil # TODO: is the logic repetitive? this else is the same with the if not fill_in_place below? No because the order matters. else: new_data = copy.deepcopy(self.data) - new_uncertainty = copy.deepcopy(self.uncertainty) + # The memo keeps deepcopy from copying this cube through the uncertainty's parent. + new_uncertainty = copy.deepcopy(self.uncertainty, {id(self): None}) new_mask = False if unmask else copy.deepcopy(self.mask) # self.mask still exists. masked = ( diff --git a/ndcube/tests/test_ndcube_arithmetic.py b/ndcube/tests/test_ndcube_arithmetic.py index 2805431a7..09c4bd4b1 100644 --- a/ndcube/tests/test_ndcube_arithmetic.py +++ b/ndcube/tests/test_ndcube_arithmetic.py @@ -434,9 +434,9 @@ def test_cube_arithmetic_neg(ndcube_2d_ln_lt_units): ) -@pytest.mark.parametrize(("operation", "limit"), [(operator.neg, 2.5), (lambda cube: cube * 2, 3.5)], - ids=["neg", "mul"]) -def test_cube_arithmetic_does_not_copy_cube(operation, limit): +@pytest.mark.parametrize("operation", [operator.neg, lambda cube: cube * 2, lambda cube: cube.fill_masked(0)], + ids=["neg", "mul", "fill_masked"]) +def test_cube_arithmetic_does_not_copy_cube(operation): shape = (2500, 2500) # 50 MB of float64 cube = NDCube(np.ones(shape), wcs=astropy.wcs.WCS(naxis=2), unit=u.ct, uncertainty=StdDevUncertainty(np.ones(shape)), meta={"a": 1}) @@ -449,10 +449,11 @@ def test_cube_arithmetic_does_not_copy_cube(operation, limit): peak = tracemalloc.get_traced_memory()[1] finally: tracemalloc.stop() - # The new data and uncertainty take 100 MB for neg and 150 MB for mul. - # Copying the uncertainty used to copy the cube's data through parent_nddata (50 MB). - # Copying extra_coords or global_coords used to copy the cube with its uncertainty (100 MB). - assert peak < limit * cube.data.nbytes + # The new data and uncertainty take 100 MB. + # Copying the uncertainty used to copy the cube's data through parent_nddata (50 MB), + # copying extra_coords or global_coords the cube with its uncertainty (100 MB), + # and mul copied its new uncertainty array again (50 MB). + assert peak < 2.5 * cube.data.nbytes assert new_cube.extra_coords._ndcube is new_cube assert new_cube.global_coords._ndcube is new_cube From 7cd9d31485ec1ee0d0c1ced313d9c26461b3f830 Mon Sep 17 00:00:00 2001 From: Nabil Freij Date: Wed, 21 Oct 2026 12:29:05 -0700 Subject: [PATCH 4/8] fix: avoid copying coordinate parents --- changelog/+new-instance-copies.bugfix.rst | 1 - changelog/996.bugfix.rst | 2 + ndcube/extra_coords/extra_coords.py | 11 +++++ ndcube/global_coords.py | 10 ++++ ndcube/ndcube.py | 8 ++-- ndcube/tests/test_ndcube.py | 57 ++++++++++++++++++++++- ndcube/tests/test_ndcube_arithmetic.py | 30 ++++++------ 7 files changed, 95 insertions(+), 24 deletions(-) delete mode 100644 changelog/+new-instance-copies.bugfix.rst create mode 100644 changelog/996.bugfix.rst diff --git a/changelog/+new-instance-copies.bugfix.rst b/changelog/+new-instance-copies.bugfix.rst deleted file mode 100644 index b51189104..000000000 --- a/changelog/+new-instance-copies.bugfix.rst +++ /dev/null @@ -1 +0,0 @@ -Arithmetic on an `~ndcube.NDCube`, `ndcube.NDCube.to` and `ndcube.NDCube.fill_masked` no longer make a temporary copy of the whole cube, data included, when copying the uncertainty, extra coordinates and global coordinates for the result, and no longer copy the attributes the operation replaces. diff --git a/changelog/996.bugfix.rst b/changelog/996.bugfix.rst new file mode 100644 index 000000000..9444c65d3 --- /dev/null +++ b/changelog/996.bugfix.rst @@ -0,0 +1,2 @@ +Arithmetic on an `~ndcube.NDCube`, `~ndcube.NDCube.to` and `~ndcube.NDCube.fill_masked` no longer make a temporary copy of the whole cube, data included, when copying the uncertainty, extra coordinates and global coordinates for the result, and no longer copy the attributes the operation replaces. +Deep copying `~ndcube.ExtraCoords` and `~ndcube.GlobalCoords` now copies their coordinate state without copying or traversing their parent cube. diff --git a/ndcube/extra_coords/extra_coords.py b/ndcube/extra_coords/extra_coords.py index 47c9588c3..c336980f1 100644 --- a/ndcube/extra_coords/extra_coords.py +++ b/ndcube/extra_coords/extra_coords.py @@ -1,4 +1,5 @@ import abc +import copy from collections.abc import Iterable from functools import partial, reduce from numbers import Integral @@ -156,6 +157,16 @@ def __init__(self, ndcube=None): # We need a reference to the parent NDCube self._ndcube = ndcube + def __deepcopy__(self, memo): + new = copy.copy(self) + memo[id(self)] = new + for key, value in vars(self).items(): + if key != "_ndcube": + setattr(new, key, copy.deepcopy(value, memo)) + # Reuse the parent, or its existing copy when copying an entire cube. + new._ndcube = memo.get(id(self._ndcube), self._ndcube) + return new + @classmethod def from_lookup_tables(cls, names, pixel_dimensions, lookup_tables, physical_types=None): """ diff --git a/ndcube/global_coords.py b/ndcube/global_coords.py index 3514a1799..c6339f620 100644 --- a/ndcube/global_coords.py +++ b/ndcube/global_coords.py @@ -88,6 +88,16 @@ def __init__(self, ndcube=None): self._ndcube = ndcube self._internal_coords = OrderedDict() + def __deepcopy__(self, memo): + new = copy.copy(self) + memo[id(self)] = new + for key, value in vars(self).items(): + if key != "_ndcube": + setattr(new, key, copy.deepcopy(value, memo)) + # Reuse the parent, or its existing copy when copying an entire cube. + new._ndcube = memo.get(id(self._ndcube), self._ndcube) + return new + @staticmethod def _convert_dropped_to_internal(dropped_dimensions): """ diff --git a/ndcube/ndcube.py b/ndcube/ndcube.py index b152630c2..8da67e60a 100644 --- a/ndcube/ndcube.py +++ b/ndcube/ndcube.py @@ -954,8 +954,7 @@ def plot(self, *args, **kwargs): return self.plotter.plot(*args, **kwargs) def _new_instance(self, **kwargs): - # The uncertainty, extra coords and global coords refer back to this cube. - # Map it in the memo so deepcopy does not copy the whole cube through them. + # Astropy's uncertainty copy state includes its parent cube. memo = {id(self): None} keys = ('unit', 'wcs', 'mask', 'meta', 'uncertainty', 'psf') new_kwargs = {k: deepcopy(getattr(self, k, None), memo) for k in keys if k not in kwargs} @@ -967,11 +966,10 @@ def _new_instance(self, **kwargs): full_kwargs['data'] = self.data full_kwargs.update(kwargs) new_cube = type(self)(**full_kwargs) - memo[id(self)] = new_cube if self.extra_coords is not None: - new_cube._extra_coords = deepcopy(self.extra_coords, memo) + new_cube._extra_coords = deepcopy(self.extra_coords) if self.global_coords is not None: - new_cube._global_coords = deepcopy(self.global_coords, memo) + new_cube._global_coords = deepcopy(self.global_coords) return new_cube def __neg__(self): diff --git a/ndcube/tests/test_ndcube.py b/ndcube/tests/test_ndcube.py index 7719b842c..aaa5d4119 100644 --- a/ndcube/tests/test_ndcube.py +++ b/ndcube/tests/test_ndcube.py @@ -1,4 +1,6 @@ +from copy import deepcopy from inspect import signature +from unittest.mock import Mock import numpy as np import pytest @@ -9,7 +11,7 @@ from astropy.wcs.wcsapi import BaseHighLevelWCS from astropy.wcs.wcsapi.wrappers import SlicedLowLevelWCS -from ndcube import NDCube +from ndcube import ExtraCoords, GlobalCoords, NDCube from ndcube.tests import helpers @@ -61,6 +63,54 @@ def test_initialize_with_extra_global_coords(ndcube_3d_ln_lt_l_ec_all_axes): helpers.assert_global_coords_equal(new_cube_copy.global_coords, gc) +@pytest.mark.parametrize("coords_name", ["extra_coords", "global_coords"]) +def test_deepcopy_coords_does_not_copy_parent(ndcube_3d_l_ln_lt_ectime, coords_name, monkeypatch): + cube = ndcube_3d_l_ln_lt_ectime + cube.global_coords.add("distance", "pos.distance", 1 * u.m) + coords = getattr(cube, coords_name) + monkeypatch.setattr(NDCube, "__deepcopy__", Mock(side_effect=AssertionError("Parent cube was copied")), raising=False) + + copied = deepcopy(coords) + + assert copied is not coords + assert copied._ndcube is cube + if coords_name == "extra_coords": + helpers.assert_extra_coords_equal(copied, coords) + copied._lookup_tables[0][1].table[0] += 1 * u.s + assert copied._lookup_tables[0][1].table[0] != coords._lookup_tables[0][1].table[0] + else: + helpers.assert_global_coords_equal(copied, coords) + copied["distance"][...] = 2 * u.m + assert coords["distance"] == 1 * u.m + + +@pytest.mark.parametrize("coords_type", [ExtraCoords, GlobalCoords]) +def test_deepcopy_coords_cycles(coords_type): + coords = coords_type() + coords.cycle = [coords] + coords.alias = coords.cycle + + copied = deepcopy(coords) + + assert copied.cycle is not coords.cycle + assert copied.cycle[0] is copied + assert copied.alias is copied.cycle + + +def test_deepcopy_cube_keeps_coords_linked(ndcube_3d_l_ln_lt_ectime): + cube = ndcube_3d_l_ln_lt_ectime + cube.global_coords.add("distance", "pos.distance", 1 * u.m) + + copied = deepcopy(cube) + + helpers.assert_cubes_equal(copied, cube, check_uncertainty_values=True) + helpers.assert_global_coords_equal(copied.global_coords, cube.global_coords) + assert copied.extra_coords._ndcube is copied + assert copied.global_coords._ndcube is copied + assert cube.extra_coords._ndcube is cube + assert cube.global_coords._ndcube is cube + + def test_wcs_type_after_init(ndcube_3d_ln_lt_l, wcs_3d_l_lt_ln): # Generate a low level WCS slices = np.s_[:, :, 0] @@ -233,12 +283,15 @@ def test_fill_masked_fill_in_place_true(ndc, fill_value, uncertainty_fill_value, ], indirect=("ndc", "expected_cube") ) -def test_fill_masked_fill_in_place_false(ndc, fill_value, uncertainty_fill_value, unmask, expected_cube): +def test_fill_masked_fill_in_place_false(ndc, fill_value, uncertainty_fill_value, unmask, expected_cube, monkeypatch): + monkeypatch.setattr(NDCube, "__deepcopy__", Mock(side_effect=AssertionError("Parent cube was copied")), raising=False) # compare the expected cube with the cube saved in the new place # perform the fill_masked method on the fixture, using parametrized as parameters. filled_cube = ndc.fill_masked(fill_value, uncertainty_fill_value, unmask, fill_in_place=False) helpers.assert_cubes_equal(filled_cube, expected_cube, check_uncertainty_values=True) + assert filled_cube.uncertainty.parent_nddata is filled_cube + assert ndc.uncertainty.parent_nddata is ndc @pytest.mark.parametrize( ("ndc", "fill_value", "uncertainty_fill_value", "unmask"), diff --git a/ndcube/tests/test_ndcube_arithmetic.py b/ndcube/tests/test_ndcube_arithmetic.py index 09c4bd4b1..b16235940 100644 --- a/ndcube/tests/test_ndcube_arithmetic.py +++ b/ndcube/tests/test_ndcube_arithmetic.py @@ -1,5 +1,4 @@ import operator -import tracemalloc import numpy as np import pytest @@ -434,28 +433,27 @@ def test_cube_arithmetic_neg(ndcube_2d_ln_lt_units): ) -@pytest.mark.parametrize("operation", [operator.neg, lambda cube: cube * 2, lambda cube: cube.fill_masked(0)], - ids=["neg", "mul", "fill_masked"]) +@pytest.mark.parametrize("operation", [operator.neg, lambda cube: cube * 2, lambda cube: cube.to(u.mct)], + ids=["neg", "mul", "to"]) def test_cube_arithmetic_does_not_copy_cube(operation): - shape = (2500, 2500) # 50 MB of float64 - cube = NDCube(np.ones(shape), wcs=astropy.wcs.WCS(naxis=2), unit=u.ct, + class NoDeepCopy(np.ndarray): + def __deepcopy__(self, memo): + raise AssertionError("Data array was deep copied") + + shape = (3, 4) + cube = NDCube(np.ones(shape).view(NoDeepCopy), wcs=astropy.wcs.WCS(naxis=2), unit=u.ct, uncertainty=StdDevUncertainty(np.ones(shape)), meta={"a": 1}) cube.extra_coords.add("time", 0, np.arange(shape[0]) * u.s) cube.global_coords.add("name", "custom:physical_type", 1 * u.s) - tracemalloc.start() - try: - new_cube = operation(cube) - peak = tracemalloc.get_traced_memory()[1] - finally: - tracemalloc.stop() - # The new data and uncertainty take 100 MB. - # Copying the uncertainty used to copy the cube's data through parent_nddata (50 MB), - # copying extra_coords or global_coords the cube with its uncertainty (100 MB), - # and mul copied its new uncertainty array again (50 MB). - assert peak < 2.5 * cube.data.nbytes + new_cube = operation(cube) + assert new_cube.extra_coords._ndcube is new_cube assert new_cube.global_coords._ndcube is new_cube + assert new_cube.uncertainty.parent_nddata is new_cube + assert cube.extra_coords._ndcube is cube + assert cube.global_coords._ndcube is cube + assert cube.uncertainty.parent_nddata is cube new_cube.meta["a"] = 2 new_cube.uncertainty.array[0, 0] = 5 From 0638858665b0a465c55d4391919b975161fa07dd Mon Sep 17 00:00:00 2001 From: Stuart Mumford Date: Thu, 22 Oct 2026 16:18:43 +0100 Subject: [PATCH 5/8] Also support deepcopying Plotter without copying parent --- ndcube/tests/test_ndcube.py | 47 ++++++++++++++++++++++++++++++++++++ ndcube/visualization/base.py | 11 +++++++++ 2 files changed, 58 insertions(+) diff --git a/ndcube/tests/test_ndcube.py b/ndcube/tests/test_ndcube.py index aaa5d4119..b31f9b5e0 100644 --- a/ndcube/tests/test_ndcube.py +++ b/ndcube/tests/test_ndcube.py @@ -13,6 +13,14 @@ from ndcube import ExtraCoords, GlobalCoords, NDCube from ndcube.tests import helpers +from ndcube.visualization import BasePlotter + + +class _DummyPlotter(BasePlotter): + """A concrete plotter for testing ``BasePlotter`` without a matplotlib dependency.""" + + def plot(self, *args, **kwargs): + pass def test_array_axis_physical_types(ndcube_3d_ln_lt_l): @@ -111,6 +119,45 @@ def test_deepcopy_cube_keeps_coords_linked(ndcube_3d_l_ln_lt_ectime): assert cube.global_coords._ndcube is cube +def test_deepcopy_plotter_does_not_copy_parent(ndcube_3d_l_ln_lt_ectime, monkeypatch): + cube = ndcube_3d_l_ln_lt_ectime + plotter = _DummyPlotter(cube) + plotter.canvas = [1, 2, 3] + monkeypatch.setattr(NDCube, "__deepcopy__", Mock(side_effect=AssertionError("Parent cube was copied")), raising=False) + + copied = deepcopy(plotter) + + assert copied is not plotter + assert copied._ndcube is cube + assert copied.canvas == [1, 2, 3] + copied.canvas.append(4) + assert plotter.canvas == [1, 2, 3] + + +def test_deepcopy_plotter_cycles(): + plotter = _DummyPlotter() + plotter.cycle = [plotter] + plotter.alias = plotter.cycle + + copied = deepcopy(plotter) + + assert copied.cycle is not plotter.cycle + assert copied.cycle[0] is copied + assert copied.alias is copied.cycle + + +def test_deepcopy_cube_keeps_plotter_linked(ndcube_3d_l_ln_lt_ectime): + cube = ndcube_3d_l_ln_lt_ectime + cube._plotter = _DummyPlotter(cube) + + copied = deepcopy(cube) + + helpers.assert_cubes_equal(copied, cube, check_uncertainty_values=True) + assert copied.plotter is not cube.plotter + assert copied.plotter._ndcube is copied + assert cube.plotter._ndcube is cube + + def test_wcs_type_after_init(ndcube_3d_ln_lt_l, wcs_3d_l_lt_ln): # Generate a low level WCS slices = np.s_[:, :, 0] diff --git a/ndcube/visualization/base.py b/ndcube/visualization/base.py index d981242b0..2d0ee3b35 100644 --- a/ndcube/visualization/base.py +++ b/ndcube/visualization/base.py @@ -1,4 +1,5 @@ import abc +import copy class BasePlotter(abc.ABC): @@ -21,3 +22,13 @@ def plot(self, *args, **kwargs): The ``plot()`` method **should** accept ``**kwargs``. """ + + def __deepcopy__(self, memo): + new = copy.copy(self) + memo[id(self)] = new + for key, value in vars(self).items(): + if key != "_ndcube": + setattr(new, key, copy.deepcopy(value, memo)) + # Reuse the parent, or its existing copy when copying an entire cube. + new._ndcube = memo.get(id(self._ndcube), self._ndcube) + return new From 861fccb81daf23ec91ce7ca6e977ab9a5301d3cc Mon Sep 17 00:00:00 2001 From: Stuart Mumford Date: Thu, 22 Oct 2026 16:22:14 +0100 Subject: [PATCH 6/8] Apply batched suggestions from code review Co-authored-by: Stuart Mumford --- changelog/996.bugfix.rst | 4 ++-- ndcube/tests/test_ndcube.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/changelog/996.bugfix.rst b/changelog/996.bugfix.rst index 9444c65d3..59e3ee7e8 100644 --- a/changelog/996.bugfix.rst +++ b/changelog/996.bugfix.rst @@ -1,2 +1,2 @@ -Arithmetic on an `~ndcube.NDCube`, `~ndcube.NDCube.to` and `~ndcube.NDCube.fill_masked` no longer make a temporary copy of the whole cube, data included, when copying the uncertainty, extra coordinates and global coordinates for the result, and no longer copy the attributes the operation replaces. -Deep copying `~ndcube.ExtraCoords` and `~ndcube.GlobalCoords` now copies their coordinate state without copying or traversing their parent cube. +Deep copying `~ndcube.ExtraCoords`, `~ndcube.GlobalCoords`, and `~ndcube.visualization.BasePlotter` now copy without copying their parent cube. +This reduces memory use when doing operations such as arithmetic or `~ndcube.NDCube.to` etc. diff --git a/ndcube/tests/test_ndcube.py b/ndcube/tests/test_ndcube.py index b31f9b5e0..39afe6f80 100644 --- a/ndcube/tests/test_ndcube.py +++ b/ndcube/tests/test_ndcube.py @@ -86,7 +86,7 @@ def test_deepcopy_coords_does_not_copy_parent(ndcube_3d_l_ln_lt_ectime, coords_n helpers.assert_extra_coords_equal(copied, coords) copied._lookup_tables[0][1].table[0] += 1 * u.s assert copied._lookup_tables[0][1].table[0] != coords._lookup_tables[0][1].table[0] - else: + if coords_name == "global_coords": helpers.assert_global_coords_equal(copied, coords) copied["distance"][...] = 2 * u.m assert coords["distance"] == 1 * u.m From 1f941c1089a4313c1228a9ac1c649ecfc61afc85 Mon Sep 17 00:00:00 2001 From: Nabil Freij Date: Thu, 22 Oct 2026 16:53:34 -0700 Subject: [PATCH 7/8] Share the deepcopy helper and tidy comments and tests from review --- changelog/996.bugfix.rst | 4 ++-- ndcube/conftest.py | 7 +++++++ ndcube/extra_coords/extra_coords.py | 13 ++----------- ndcube/global_coords.py | 11 ++--------- ndcube/ndcube.py | 5 +++-- ndcube/tests/test_ndcube.py | 24 +++++++++++++++--------- ndcube/utils/misc.py | 19 +++++++++++++++++++ ndcube/visualization/base.py | 13 +++---------- 8 files changed, 53 insertions(+), 43 deletions(-) diff --git a/changelog/996.bugfix.rst b/changelog/996.bugfix.rst index 59e3ee7e8..428f9644d 100644 --- a/changelog/996.bugfix.rst +++ b/changelog/996.bugfix.rst @@ -1,2 +1,2 @@ -Deep copying `~ndcube.ExtraCoords`, `~ndcube.GlobalCoords`, and `~ndcube.visualization.BasePlotter` now copy without copying their parent cube. -This reduces memory use when doing operations such as arithmetic or `~ndcube.NDCube.to` etc. +Deep copies of `~ndcube.ExtraCoords`, `~ndcube.GlobalCoords` and `~ndcube.visualization.BasePlotter` no longer copy their parent cube. +This reduces memory use in operations such as arithmetic and `~ndcube.NDCube.to`. diff --git a/ndcube/conftest.py b/ndcube/conftest.py index 6e591ebaf..2afa617a0 100644 --- a/ndcube/conftest.py +++ b/ndcube/conftest.py @@ -3,6 +3,7 @@ predictable NDCube objects. """ import logging +from unittest.mock import Mock import numpy as np import pytest @@ -1328,6 +1329,12 @@ def lut_1d_wave(): return QuantityTableCoordinate(range(10) * u.nm) +@pytest.fixture +def forbid_cube_deepcopy(monkeypatch): + """Make any deep copy of an `~ndcube.NDCube` fail the test.""" + monkeypatch.setattr(NDCube, "__deepcopy__", Mock(side_effect=AssertionError("Parent cube was copied")), raising=False) + + def pytest_runtest_teardown(item): # Clear the pyplot figure stack if it is not empty after the test # You can see these log messages by passing "-o log_cli=true" to pytest on the command line diff --git a/ndcube/extra_coords/extra_coords.py b/ndcube/extra_coords/extra_coords.py index c336980f1..01a2fa687 100644 --- a/ndcube/extra_coords/extra_coords.py +++ b/ndcube/extra_coords/extra_coords.py @@ -1,5 +1,4 @@ import abc -import copy from collections.abc import Iterable from functools import partial, reduce from numbers import Integral @@ -15,7 +14,7 @@ from astropy.wcs.wcsapi.high_level_wcs_wrapper import HighLevelWCSWrapper from astropy.wcs.wcsapi.wrappers.sliced_wcs import SlicedLowLevelWCS -from ndcube.utils.misc import _simplify_basic_index +from ndcube.utils.misc import _deepcopy_without_parent, _simplify_basic_index from ndcube.utils.wcs import convert_between_array_and_pixel_axes from ndcube.wcs.wrappers import CompoundLowLevelWCS, ResampledLowLevelWCS from .table_coord import ( @@ -157,15 +156,7 @@ def __init__(self, ndcube=None): # We need a reference to the parent NDCube self._ndcube = ndcube - def __deepcopy__(self, memo): - new = copy.copy(self) - memo[id(self)] = new - for key, value in vars(self).items(): - if key != "_ndcube": - setattr(new, key, copy.deepcopy(value, memo)) - # Reuse the parent, or its existing copy when copying an entire cube. - new._ndcube = memo.get(id(self._ndcube), self._ndcube) - return new + __deepcopy__ = _deepcopy_without_parent @classmethod def from_lookup_tables(cls, names, pixel_dimensions, lookup_tables, physical_types=None): diff --git a/ndcube/global_coords.py b/ndcube/global_coords.py index c6339f620..af6355844 100644 --- a/ndcube/global_coords.py +++ b/ndcube/global_coords.py @@ -10,6 +10,7 @@ from astropy.wcs.wcsapi.high_level_api import default_order from astropy.wcs.wcsapi.utils import deserialize_class +from ndcube.utils.misc import _deepcopy_without_parent from ndcube.utils.wcs import validate_physical_types @@ -88,15 +89,7 @@ def __init__(self, ndcube=None): self._ndcube = ndcube self._internal_coords = OrderedDict() - def __deepcopy__(self, memo): - new = copy.copy(self) - memo[id(self)] = new - for key, value in vars(self).items(): - if key != "_ndcube": - setattr(new, key, copy.deepcopy(value, memo)) - # Reuse the parent, or its existing copy when copying an entire cube. - new._ndcube = memo.get(id(self._ndcube), self._ndcube) - return new + __deepcopy__ = _deepcopy_without_parent @staticmethod def _convert_dropped_to_internal(dropped_dimensions): diff --git a/ndcube/ndcube.py b/ndcube/ndcube.py index 8da67e60a..b1b0d75b5 100644 --- a/ndcube/ndcube.py +++ b/ndcube/ndcube.py @@ -954,7 +954,8 @@ def plot(self, *args, **kwargs): return self.plotter.plot(*args, **kwargs) def _new_instance(self, **kwargs): - # Astropy's uncertainty copy state includes its parent cube. + # Astropy's NDUncertainty deep copies its parent_nddata, i.e. this whole cube. + # Seeding the memo makes that parent copy as None; the new cube then becomes its parent. memo = {id(self): None} keys = ('unit', 'wcs', 'mask', 'meta', 'uncertainty', 'psf') new_kwargs = {k: deepcopy(getattr(self, k, None), memo) for k in keys if k not in kwargs} @@ -1463,7 +1464,7 @@ def fill_masked(self, fill_value, uncertainty_fill_value=None, unmask=False, fil # TODO: is the logic repetitive? this else is the same with the if not fill_in_place below? No because the order matters. else: new_data = copy.deepcopy(self.data) - # The memo keeps deepcopy from copying this cube through the uncertainty's parent. + # The memo stops deepcopy copying this cube via the uncertainty's parent; see _new_instance. new_uncertainty = copy.deepcopy(self.uncertainty, {id(self): None}) new_mask = False if unmask else copy.deepcopy(self.mask) # self.mask still exists. diff --git a/ndcube/tests/test_ndcube.py b/ndcube/tests/test_ndcube.py index 39afe6f80..252a41da1 100644 --- a/ndcube/tests/test_ndcube.py +++ b/ndcube/tests/test_ndcube.py @@ -1,6 +1,5 @@ from copy import deepcopy from inspect import signature -from unittest.mock import Mock import numpy as np import pytest @@ -72,11 +71,10 @@ def test_initialize_with_extra_global_coords(ndcube_3d_ln_lt_l_ec_all_axes): @pytest.mark.parametrize("coords_name", ["extra_coords", "global_coords"]) -def test_deepcopy_coords_does_not_copy_parent(ndcube_3d_l_ln_lt_ectime, coords_name, monkeypatch): +def test_deepcopy_coords_does_not_copy_parent(ndcube_3d_l_ln_lt_ectime, coords_name, forbid_cube_deepcopy): cube = ndcube_3d_l_ln_lt_ectime cube.global_coords.add("distance", "pos.distance", 1 * u.m) coords = getattr(cube, coords_name) - monkeypatch.setattr(NDCube, "__deepcopy__", Mock(side_effect=AssertionError("Parent cube was copied")), raising=False) copied = deepcopy(coords) @@ -84,8 +82,8 @@ def test_deepcopy_coords_does_not_copy_parent(ndcube_3d_l_ln_lt_ectime, coords_n assert copied._ndcube is cube if coords_name == "extra_coords": helpers.assert_extra_coords_equal(copied, coords) - copied._lookup_tables[0][1].table[0] += 1 * u.s - assert copied._lookup_tables[0][1].table[0] != coords._lookup_tables[0][1].table[0] + copied.add("exposure", 1, np.arange(5) * u.s) + assert list(coords.keys()) == ["time"] if coords_name == "global_coords": helpers.assert_global_coords_equal(copied, coords) copied["distance"][...] = 2 * u.m @@ -119,11 +117,10 @@ def test_deepcopy_cube_keeps_coords_linked(ndcube_3d_l_ln_lt_ectime): assert cube.global_coords._ndcube is cube -def test_deepcopy_plotter_does_not_copy_parent(ndcube_3d_l_ln_lt_ectime, monkeypatch): +def test_deepcopy_plotter_does_not_copy_parent(ndcube_3d_l_ln_lt_ectime, forbid_cube_deepcopy): cube = ndcube_3d_l_ln_lt_ectime plotter = _DummyPlotter(cube) plotter.canvas = [1, 2, 3] - monkeypatch.setattr(NDCube, "__deepcopy__", Mock(side_effect=AssertionError("Parent cube was copied")), raising=False) copied = deepcopy(plotter) @@ -146,6 +143,16 @@ def test_deepcopy_plotter_cycles(): assert copied.alias is copied.cycle +def test_deepcopy_plotter_without_base_init(): + class NoInitPlotter(_DummyPlotter): + def __init__(self): + pass + + copied = deepcopy(NoInitPlotter()) + + assert not hasattr(copied, "_ndcube") + + def test_deepcopy_cube_keeps_plotter_linked(ndcube_3d_l_ln_lt_ectime): cube = ndcube_3d_l_ln_lt_ectime cube._plotter = _DummyPlotter(cube) @@ -330,8 +337,7 @@ def test_fill_masked_fill_in_place_true(ndc, fill_value, uncertainty_fill_value, ], indirect=("ndc", "expected_cube") ) -def test_fill_masked_fill_in_place_false(ndc, fill_value, uncertainty_fill_value, unmask, expected_cube, monkeypatch): - monkeypatch.setattr(NDCube, "__deepcopy__", Mock(side_effect=AssertionError("Parent cube was copied")), raising=False) +def test_fill_masked_fill_in_place_false(ndc, fill_value, uncertainty_fill_value, unmask, expected_cube, forbid_cube_deepcopy): # compare the expected cube with the cube saved in the new place # perform the fill_masked method on the fixture, using parametrized as parameters. diff --git a/ndcube/utils/misc.py b/ndcube/utils/misc.py index eb19a03e8..aa24c348a 100644 --- a/ndcube/utils/misc.py +++ b/ndcube/utils/misc.py @@ -1,3 +1,4 @@ +import copy import numbers from collections.abc import Sequence from types import EllipsisType @@ -44,6 +45,24 @@ def convert_quantities_to_units(coords, units): for coord, unit in zip(coords, units)] +def _deepcopy_without_parent(obj, memo): + """ + Deep copy an object without copying the parent cube it links to through ``_ndcube``. + + Assign this as ``__deepcopy__`` on the class. + """ + new = copy.copy(obj) + memo[id(obj)] = new + for key, value in vars(obj).items(): + if key != "_ndcube": + setattr(new, key, copy.deepcopy(value, memo)) + # A plotter subclass that skips BasePlotter.__init__ has no _ndcube. + if hasattr(obj, "_ndcube"): + # Use the cube's copy if deepcopy reached the cube first, otherwise keep the original cube. + new._ndcube = memo.get(id(obj._ndcube), obj._ndcube) + return new + + if minversion("astropy", "8.0.2"): from astropy.utils.shapes import simplify_basic_index as _simplify_basic_index else: diff --git a/ndcube/visualization/base.py b/ndcube/visualization/base.py index 2d0ee3b35..76aa3ab5b 100644 --- a/ndcube/visualization/base.py +++ b/ndcube/visualization/base.py @@ -1,5 +1,6 @@ import abc -import copy + +from ndcube.utils.misc import _deepcopy_without_parent class BasePlotter(abc.ABC): @@ -23,12 +24,4 @@ def plot(self, *args, **kwargs): The ``plot()`` method **should** accept ``**kwargs``. """ - def __deepcopy__(self, memo): - new = copy.copy(self) - memo[id(self)] = new - for key, value in vars(self).items(): - if key != "_ndcube": - setattr(new, key, copy.deepcopy(value, memo)) - # Reuse the parent, or its existing copy when copying an entire cube. - new._ndcube = memo.get(id(self._ndcube), self._ndcube) - return new + __deepcopy__ = _deepcopy_without_parent From 81cc996905623f853719689cb8f88e65fe310ff0 Mon Sep 17 00:00:00 2001 From: Alasdair Wilson <60351846+alasdairwilson@users.noreply.github.com> Date: Fri, 23 Oct 2026 14:02:32 +0100 Subject: [PATCH 8/8] Update ndcube/ndcube.py --- ndcube/ndcube.py | 1 + 1 file changed, 1 insertion(+) diff --git a/ndcube/ndcube.py b/ndcube/ndcube.py index b1b0d75b5..26f249f2e 100644 --- a/ndcube/ndcube.py +++ b/ndcube/ndcube.py @@ -1104,6 +1104,7 @@ def __mul__(self, value): else: kwargs["unit"] = self.unit kwargs["data"] = self.data * value + # The array is already new, don't copy it again kwargs["uncertainty"] = (type(self.uncertainty)(self.uncertainty.array * value, copy=False) if self.uncertainty is not None else None) return self._new_instance(**kwargs)