Fix problem with (deep)copy of TextPath #20921
Conversation
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__ on TextPath.
It may also be worth looking into using Path._fast_from_codes_and_verts in Path.__deepcopy__ + patching on some other state + extending it in TextPath 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 :-) |
| from matplotlib.textpath import TextPath | ||
|
|
||
|
|
||
| def test_set_size(): |
There was a problem hiding this comment.
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.
I have added a simple docstring. I have revisited the test and I think that it tests the required functionality.
The problem is that deepcopy of TextPath calls deepcopy of Path. In turn `Path` utilizes `super().__init__` for creating a new instance. However, `Path.__init__ and TextPath.__init__ are completely different. Moreover, `TextPath.__init__` converts some arguments directly to list of vertices, hence it is not possible to create a new instance of `TextPath` from its members. Additionally, there is a problem with caching. When copying, the vertices can be copied uncached, which causes discrepancies between members of copy and original members (recaching creates new members). Therefore validation of cache was removed and new vertices are generated at every size set.
This reverts commit 7c152aa.
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.
Do you need this as a public method? I'd say copy.deepcopy(textpath) is good enough (and in fact, per https://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.
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 :) |
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).The text was updated successfully, but these errors were encountered: