Skip to content

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

Merged
merged 1 commit into from
Jan 30, 2020
Merged

Cleanup list copying #16327

merged 1 commit into from
Jan 30, 2020

Conversation

timhoffm
Copy link
Member

@timhoffm timhoffm commented Jan 25, 2020

PR Summary

  • use list.copy() instead of list[:]. This is supported since Python 3.3 (and also works for numpy arrays).
  • Two places actually did not need a copy because the resulting list is not modified.
  • some further cleanup in draw_gouraud_triangle

@timhoffm timhoffm added this to the v3.3.0 milestone Jan 25, 2020
@@ -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[:]
Copy link
Contributor

@anntzer anntzer Jan 25, 2020

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)

Copy link
Member Author

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.

Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Contributor

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.

Copy link
Member Author

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).

Copy link
Contributor

@anntzer anntzer left a comment

Choose a reason for hiding this comment

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

postci

@jklymak jklymak merged commit ab098f2 into matplotlib:master Jan 30, 2020
@timhoffm timhoffm deleted the list-copy branch February 1, 2020 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants