Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Improved implementation of Path.copy and deepcopy#20731

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
timhoffm merged 2 commits intomatplotlib:masterfromdeep-jkl:path-copy
Jul 25, 2021

Conversation

deep-jkl
Copy link
Contributor

@deep-jkldeep-jkl commentedJul 24, 2021
edited
Loading

PR Summary

The original Path.copy raised RecursionError, the new implementation mimicks deepcopy, but Path members are shared.

Path tests were updated to utilize member functions copy and deepcopy instead of builtin copy functional protocol.

PR Checklist

  • Has pytest style unit tests (andpytest passes).
  • IsFlake 8 compliant (runflake8 on changed files to check).
  • [n/a] New features are documented, with examples if plot related.
  • [n/a] Documentation is sphinx and numpydoc compliant (the docs shouldbuild without error).
  • [n/a] Conforms to Matplotlib style conventions (installflake8-docstrings and runflake8 --docstring-convention=all).
  • [n/a] New features have an entry indoc/users/next_whats_new/ (follow instructions in README.rst there).
  • [n/a] API changes documented indoc/api/next_api_changes/ (follow instructions in README.rst there).

The original Path.copy raised RecursionError, new implementation mimicksdeepcopy, creating shared members.Path tests were updated to utilize member functions copy and deepcopyinstead of builtin copy functional protocol.
try:
codes = self.codes
except AttributeError:
codes = None
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to check for attributeerror, codes is always there (it can be None though; the AttributeError in deepcopy is to handle the codes is None case).

Actually I think the default implementation of__copy__ will just work, i.e. removing the implementation of__copy__ and just havingdef copy(x): return copy.copy(x) would work? And likewise for__deepcopy__?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I agree thatcopy is redundant and copy.copy will work, it just didn't in previous case because it was incopy call what caused infinite recurrence. On the other hand__deepcopy__ implementation is kind of optimized, isn't it? I mean that it is not recurrently calling deepcopy on each of its members as default implementation should. However, custom__deepcopy__ implementation can backfire, when we add important members and it is more code to maintain.

To conclude, there are two implementations of__deepcopy__ in the sources, and three implementations of__copy__. The other__deepcopy__ implementation is for TransformNode class and sorts out descendants (without reading through the details I assume that it implements design requirements of the whole Transform system). The other__copy__ implementations are for TransformNode class again and Colormap class, where it seems legitimate (again meeting some external requirements).

I don't see any heavy lifting done in the Path copy functions, therefore I suggest removing__copy__,__deepcopy__, and optionally the members:Path.copy(),Path.deepcopy(), and their tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure the optimization of deepcopy really matters here. So I agree with removing__copy__ and__deepcopy__;copy anddeepcopy on the other hand can stay as convenience methods (plus it avoids breaking backcompat gratuitiously, and there's precedent forcopy methods existing for convenience (e.g. onlist ornp.ndarray), and leaving the tests forcopy anddeepcopy also verify (somewhat) that using the default impls of__copy__ and__deepcopy__ is indeed semantically correct.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I have found out that__deepcopy__ implementation cannot be done by means ofcopy.deepcopy, the reason is even stated in the docstring:

The `Path` will not be readonly, even if the source `Path` is.

Some tests relies on this behavior, one example fromtest_artist.py :

@image_comparison(["clip_path_clipping"], remove_text=True)def test_clipping():    exterior = mpath.Path.unit_rectangle().deepcopy()    exterior.vertices *= 4    exterior.vertices -= 2...

The Path frommpath.Path.unit_rectangle() is readonly, but its deepcopy isn't.
I think that at this point I should just fix the shallow copy code, which throws exceptions and keep__deepcopy__ as it is, since it has some consequences.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, sure enough.

copy.deepcopy(path2)
path1_copy = path1.deepcopy()
path2_copy = path2.deepcopy()
assert(path1 is not path1_copy)
Copy link
Contributor

Choose a reason for hiding this comment

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

no parentheses around the assert

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Fixed inf765d04.

@anntzeranntzer added the PR: bugfixPull requests that fix identified bugs labelJul 24, 2021
Copy link
Contributor

@anntzeranntzer left a comment

Choose a reason for hiding this comment

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

Modulo ci.

@timhoffmtimhoffm added this to thev3.5.0 milestoneJul 25, 2021
@timhoffmtimhoffm merged commit45528c3 intomatplotlib:masterJul 25, 2021
@deep-jkldeep-jkl deleted the path-copy branchApril 13, 2022 18:31
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@anntzeranntzeranntzer approved these changes

@timhoffmtimhoffmtimhoffm approved these changes

Assignees
No one assigned
Labels
PR: bugfixPull requests that fix identified bugs
Projects
None yet
Milestone
v3.5.0
Development

Successfully merging this pull request may close these issues.

3 participants
@deep-jkl@anntzer@timhoffm

[8]ページ先頭

©2009-2025 Movatter.jp