Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Decide on correct meta copying behaviour in rebin #815

Open
Cadair opened this issue Jan 22, 2025 · 2 comments
Open

Decide on correct meta copying behaviour in rebin #815

Cadair opened this issue Jan 22, 2025 · 2 comments
Labels
Discussion An issue opened for, or undergoing discussion.

Comments

@Cadair
Copy link
Member

Cadair commented Jan 22, 2025

Describe the bug

Pre #455 we didn't copy the meta attribute at all if it wasn't rebin aware:

new_meta = self.meta

post #455 we deepcopy it:

new_meta = deepcopy(self.meta)

We had a long discussion about this in the matrix channel, but to summarise:

  • Our historical policy was that we made no assumptions about .meta so it could be any object of any arbitrary complexity. I feel like defaulting to deepcopy is not in the spirit of this policy.
  • It's safer to copy than not deepcopy because a deepcopy could use a lot of memory if a large array is in the .meta, and it's safer and easier for someone to explicitly deepcopy afterwards than it is to undo a deepcopy.
  • Users may well be surprised by the fact that the whole .meta is the same or the values of a dict as .meta are the same if we don't copy (old behaviour) or only copy.
@Cadair Cadair added the Discussion An issue opened for, or undergoing discussion. label Jan 22, 2025
@samaloney
Copy link
Contributor

So if the meta is axis-aware or not I will get different behaviour? If it is not axis-aware meta will be a reference to the original cubes meta but if it is axis aware it will be a new rebinded meta? My initial thought is a rebinded cube is a completely new cube and that it would not share anything with the original cube and if I changed the meta on the rebinded cube I would not expect it to alter the meta on the orig cube 🤯 I can see times where sharing it would be beneficial or even necessary.

I think there are cases where either is correct so maybe add an kwarg to control and default to old behaviour. In any case I think it needs to be called out in the documentation as a potential footgun.

@Cadair
Copy link
Member Author

Cadair commented Jan 22, 2025

If it is not axis-aware meta will be a reference to the original cubes meta but if it is axis aware it will be a new rebinded meta?

I am proposing to make it copy not deepcopy for what it's worth.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion An issue opened for, or undergoing discussion.
Projects
None yet
Development

No branches or pull requests

2 participants