Skip to content

Support deepcopying without copying references to the parent cube - #996

Open
nabobalis wants to merge 7 commits into
sunpy:mainfrom
nabobalis:new-instance-copies
Open

nabobalis wants to merge 7 commits into
sunpy:mainfrom
nabobalis:new-instance-copies

Conversation

@nabobalis

@nabobalis nabobalis commented Oct 20, 2026 •

Copy link
Copy Markdown
Member

PR Description

Deep copying ExtraCoords, GlobalCoords or a plotter also deep copied the parent cube, data included. _new_instance deep copies the coords and the uncertainty, whose parent_nddata is the cube, so arithmetic, to and fill_masked copied the whole cube each time, which took RAM over 20 GB on some of my cubes.

  • ExtraCoords, GlobalCoords and BasePlotter get a __deepcopy__ that doesn't copy the parent cube.
  • _new_instance and fill_masked no longer copy the cube through the uncertainty's parent_nddata, nor copy attributes the operation replaces.

AI Assistance Disclosure

AI tools were used for:

  • Code generation (e.g., when writing an implementation or fixing a bug)
  • Test/benchmark generation
  • Documentation (including examples)
  • Research and understanding
  • No AI tools were used

Regardless of AI use, the human contributor remains fully responsible for correctness, design choices, licensing compatibility, and long-term maintainability.

@alasdairwilson alasdairwilson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once more I find myself in a nabil PR that is fixing the thing I came to ndcube to fix, though in my case the issue with this was not ram usage but thread safety because copying the extracoords means deep copying the entire parent cube via this stale _ndcube which means it iterates the parent's dict while another thread is writing to it.

I think this could be more generally, i.e. not just inside _new_instance, and in a more human readable and still have equivalent fix if we patched it in a less obtuse way by simply making the deepcopy not copy the parent cube at all _ndcube from the deepcopy method on both extracoords and globalcoords...

    def __deepcopy__(self, memo):
        new = copy.copy(self)
        for key, value in vars(self).items():
            if key != "_ndcube":
                setattr(new, key, copy.deepcopy(value, memo))
        return new

or i guess you are supposed to work on the memo or something with memo[id(self)] = new first?

then no matter who or what calls deepcopy its threading safe and not memory inefficient

Comment thread ndcube/tests/test_ndcube_arithmetic.py Outdated
@Cadair

Cadair commented Oct 21, 2026

Copy link
Copy Markdown
Member

+1 for making sure any deepcopy of a NDCube works without copying the parent cube etc

@nabobalis
nabobalis force-pushed the new-instance-copies branch from 0f0c5e0 to 97d6af3 Compare October 21, 2026 19:29
@nabobalis
nabobalis marked this pull request as ready for review October 21, 2026 20:19
@Cadair Cadair changed the title New instance copies Support deepcopying without copying references to the parent cube. Oct 22, 2026
@Cadair Cadair changed the title Support deepcopying without copying references to the parent cube. Support deepcopying without copying references to the parent cube Oct 22, 2026
Comment thread ndcube/tests/test_ndcube.py Outdated
Comment thread changelog/996.bugfix.rst Outdated
Comment thread changelog/996.bugfix.rst Outdated
@Cadair
Cadair force-pushed the new-instance-copies branch from add2ea6 to 6b334ea Compare October 22, 2026 15:22
@Cadair Cadair added this to the 2.4.3 milestone Oct 22, 2026
@Cadair
Cadair requested a review from alasdairwilson October 22, 2026 15:23

@Cadair Cadair left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I hate it.

@Cadair
Cadair force-pushed the new-instance-copies branch from e0d6cce to 1f941c1 Compare October 23, 2026 07:47

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants