Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Fix problem with (deep)copy of TextPath#20921
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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.
Thank you for working on this, however I think this is the wrong fix.
__deepcopy__ should return the same type as the object being copied (I pushed a commit updating the test to check this)!. I think a better fix is to override the__deepcopy__ onTextPath.
It may also be worth looking into usingPath._fast_from_codes_and_verts inPath.__deepcopy__ + patching on some other state + extending it inTextPath to add any additional state if needed.
Anyone can dismiss this review.
Mea culpa! It was mine bad design. With the help of stack overflow I have come to another implementation. Regarding the |
I have filled an issue tracker for case that I will fail in this PR :-) |
f78af38 tofeaa882Compare| from matplotlib.textpath import TextPath | ||
| def test_set_size(): |
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.
Can this get a docstring, even if very cursory? I can't really tell what it is supposed to be testing...
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 have added a simple docstring. I have revisited the test and I think that it tests the required functionality.
feaa882 toebebec0CompareThe problem is that deepcopy of TextPath calls deepcopy of Path.In turn `Path` utilizes `super().__init__` for creating a newinstance. However, `Path.__init__ and TextPath.__init__ arecompletely different. Moreover, `TextPath.__init__` convertssome arguments directly to list of vertices, hence it is notpossible to create a new instance of `TextPath` from its members.Additionally, there is a problem with caching. When copying, thevertices can be copied uncached, which causes discrepanciesbetween members of copy and original members (recaching creates newmembers). Therefore validation of cache was removed and new verticesare generated at every size set.
This reverts commit7c152aa.
This time, revalidation is enabled. Added test for member deepcopy().
@jklymak@tacaswell can you review this again, please? |
Maybe this is fine, however, I don't understand why you want to deep copy this class in the first place to know if it is correct, and what use it will be put to. |
| setattr(new_instance, k, deepcopy(v, memo)) | ||
| return new_instance | ||
| deepcopy = __deepcopy__ |
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.
Do you need this as a public method? I'd saycopy.deepcopy(textpath) is good enough (and in fact, perhttps://github.com/matplotlib/matplotlib/pull/20921/files#r721988323, it does a bit more)?
| self._revalidate_path() | ||
| cls = self.__class__ | ||
| new_instance = cls.__new__(cls) | ||
| memo[id(self)] = new_instance |
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 don't think you need to fill in the memo yourself,copy.deepcopy already does that for you:https://github.com/python/cpython/blob/7c2a040a10654d67ff543a55858ba2d7a9f7eea8/Lib/copy.py#L175-L176
Actually, having reviewed this, I think#21280 is a better solution? |
I might done it the other way around, sorry. This requirement comes from another PR, where I have changed MarkerStyle instantiation from MarkerStyle to utilize I got into the whole copy/deepcopy mess when I unexpectedly discovered some bug see#20731 . |
This looks much better, I didn't knew that you can use |
Thanks for pointing out the original issue :) |
Uh oh!
There was an error while loading.Please reload this page.
PR Summary
The problem is that deepcopy of TextPath calls deepcopy of Path. In turn
Pathutilizessuper().__init__for creating a newinstance. However,
Path.__init__andTextPath.__init__are completely different.Related to Issue#20943
PR Checklist
pytestpasses).flake8on changed files to check).flake8-docstringsand runflake8 --docstring-convention=all).doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).