-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Cleanup list copying #16327
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
Cleanup list copying #16327
Conversation
@@ -3160,7 +3160,7 @@ def __call__(self, path, mutation_size, linewidth, | |||
if aspect_ratio is not None: | |||
# Squeeze the given height by the aspect_ratio | |||
|
|||
vertices, codes = path.vertices[:], path.codes[:] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
path_shrunk = Path(path.vertices / [1, aspect_ratio], path.codes)
?
(Actually the old code looks wrong -- it would modify path's vertices in places too)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. But I think I still need to copy the codes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, because path codes (and vertices) are explicitly immutable
.. note::
The vertices and codes arrays should be treated as
immutable -- there are a number of optimizations and assumptions
made up front in the constructor that will not change when the
data changes.
so it's fine to reuse the same array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I read this the other way round: "Don't modify vertices and codes." We assume that they stay the same, but it's not checked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not checked but what happens if someone changes it is explicitly undefined behavior, so we can assume that they are not changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO this claims that users are not allowed to change the code. But it does not guarantee that Path does not modify the input. In fact, if codes is a numpy array already, it's writable
flag is set to False.
Anyway, I don't want to go into a discussion about who owns the data. In this place, it's just reusing codes from another path, so nothing bad should happen anyway. I've removed the copy, which is consistent with the current behavior (the slice codes[:]
also shares the data).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
postci
PR Summary
list.copy()
instead oflist[:]
. This is supported since Python 3.3 (and also works for numpy arrays).draw_gouraud_triangle