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

fix: pass memo to deepcopy #1698

Merged
merged 1 commit into from
Sep 14, 2022
Merged

fix: pass memo to deepcopy #1698

merged 1 commit into from
Sep 14, 2022

Conversation

agoose77
Copy link
Collaborator

@jpivarski whoops, somehow I missed this on your PR.

Copy link
Member

@jpivarski jpivarski left a comment

Choose a reason for hiding this comment

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

Right—I can't think of a legitimate case in which an ak.Array instance is inside of the behavior dict, but this is formally correct and if such a situation does show up in the future, this implementation fixes a potential bug.

I'm setting up auto-merge.

@jpivarski jpivarski enabled auto-merge (squash) September 14, 2022 13:10
@codecov
Copy link

codecov bot commented Sep 14, 2022

Codecov Report

Merging #1698 (eef49bd) into main (e692946) will increase coverage by 0.40%.
The diff coverage is 84.45%.

❗ Current head eef49bd differs from pull request most recent head 6616438. Consider uploading reports for the commit 6616438 to get more accurate results

Additional details and impacted files
Impacted Files Coverage Δ
src/awkward/_v2/_connect/jax/__init__.py 90.47% <ø> (+1.58%) ⬆️
src/awkward/_v2/config.py 0.00% <0.00%> (ø)
src/awkward/_v2/operations/ak_broadcast_arrays.py 100.00% <ø> (+4.34%) ⬆️
src/awkward/_v2/operations/ak_transform.py 65.51% <ø> (+56.89%) ⬆️
src/awkward/_v2/types/uniontype.py 84.61% <ø> (ø)
src/awkward/_v2/behaviors/categorical.py 73.13% <60.00%> (-8.94%) ⬇️
src/awkward/_v2/contents/bitmaskedarray.py 71.42% <60.00%> (-0.11%) ⬇️
src/awkward/_v2/contents/bytemaskedarray.py 88.32% <60.00%> (-0.03%) ⬇️
src/awkward/_v2/contents/listarray.py 91.89% <60.00%> (-0.02%) ⬇️
src/awkward/_v2/contents/unmaskedarray.py 70.76% <60.00%> (-0.13%) ⬇️
... and 53 more

@agoose77 agoose77 enabled auto-merge (squash) September 14, 2022 13:26
@agoose77
Copy link
Collaborator Author

NB - this PR is now placing more importance on behavior being None if the user wants ak.behavior - now we deepcopy this, which would be expensive if we are currently passing around ak.behavior explicitly.

@jpivarski
Copy link
Member

Presumably, if someone actually says

my_new_array = copy.deepcopy(my_awk_array)

then they don't want my_new_array to share data with my_awk_array.

But, behaviors contain class objects and functions. We don't want them to be copied.

>>> import copy
>>> def some_function(x): return x
... 
>>> class SomeClass: pass
... 
>>> behavior = {"function": some_function, "class": SomeClass, some_function: "as key"}
>>> copied_behavior = copy.deepcopy(behavior)
>>> behavior["function"] is copied_behavior["function"]
True
>>> behavior["class"] is copied_behavior["class"]
True
>>> list(behavior.keys())[2]
<function some_function at 0x1057c7e20>
>>> list(behavior.keys())[2] is list(copied_behavior.keys())[2]
True

Okay: they're not.

@jpivarski jpivarski enabled auto-merge (squash) September 14, 2022 13:36
@agoose77
Copy link
Collaborator Author

Okay: they're not.

Yes, right now this would only be an issue if a user is passing an instance method (or other non-atomically copied value) into ak.behavior, e.g.

class SpecialAdder:
    def add(self, x, y):
        ...

ak.behavior[np.add, "point", "point"] = SpecialAdder().add

Whilst I don't anticipate this, I think it's worth discussing it up-front in case we this these kinds of issues later.

@jpivarski jpivarski merged commit 417af50 into main Sep 14, 2022
@jpivarski jpivarski deleted the agoose77/fix-memo-copy branch September 14, 2022 13:55
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.

2 participants