[Bug]: ValueError upon deepcopy of a Figure object #21554

Activity

1. anntzer

   added this to the v3.5.0 milestone on Nov 6, 2021
2. anntzer commented on Nov 6, 2021

   

   anntzer

   on Nov 6, 2021

   Contributor

   More actions

   My (unverified) guess is that this is a regression due to #17107 and https://bugs.python.org/issue19364), hence milestoning as 3.5.
3. dstansby commented on Nov 6, 2021

   

   dstansby

   on Nov 6, 2021

   Member

   More actions

   In 3.3.4 (ie. before #17107) this still errors, but with a different message:

   NotImplementedError: TransformNode instances can not be copied. Consider using frozen() instead.

   @tovrstra do you know if this has ever worked in Matplotlib?
4. tovrstra commented on Nov 6, 2021

   

   tovrstra

   on Nov 6, 2021

   Author

   More actions

   I only ran into this recently. (beginning of October this year) I'm not sure what would have happened with earlier versions of matplotlib.

   I'm also wondering if the operation should be supported. Would the deepcopy result in two different Figure instances with the same figure number? I'm not sure if that would make sense.
5. anntzer commented on Nov 6, 2021

   

   anntzer

   on Nov 6, 2021

   Contributor

   More actions

   Good catch @dstansby; I guess this may have never worked, sorry for the wrong alert.
6. anntzer

   removed this from the v3.5.0 milestone on Nov 6, 2021
7. QuLogic

   added a commit that references this issue on Nov 9, 2021

   Raise correct exception out of Spines.__getattr__

   ...

   This issue will close once commit a1d8b56 is merged into the 'main' branch.

   a1d8b56
8. timhoffm commented on Nov 9, 2021

   

   timhoffm

   on Nov 9, 2021

   Member

   More actions

   In 3.3.4 (ie. before #17107) this still errors, but with a different message:

   NotImplementedError: TransformNode instances can not be copied. Consider using frozen() instead.

   I had 3.1.3 around and there it's the same error.

   Should we explicitly forbid deepcopying a figure by overriding Figure.__deepcopy__. It seems to never have worked and being explicit about it doesn't hurt.

   👍React with 👍2Reacted by Jody Klymak and Toon Verstraelen
9. timhoffm

   closed this as completedin #21570on Nov 9, 2021
10. timhoffm commented on Nov 9, 2021

    

    timhoffm

    on Nov 9, 2021

    Last edited by timhoffm

    Member

    More actions

    I don't think #21570 does close this. It issues a more correct error, but deepcopy(figure) should not fail in some random internal place. Let's make it fail expicitly with NotImplementedError: Figure does not support deepcopy.

    👍React with 👍2Reacted by Toon Verstraelen and JOEL THOMAS WILSON
11. timhoffm

    reopened this on Nov 9, 2021
12. tovrstra commented on Nov 9, 2021

    

    tovrstra

    on Nov 9, 2021

    Author

    More actions

    @timhoffm That would indeed clarify the error indeed.

    I'm trying to understand if this would be the right behavior. For example, the following does work, which is almost the same as a deepcopy:

    import pickle
    import matplotlib.pyplot as plt
    fig1, ax = plt.subplots()
    print(fig1.number)
    fig2 = pickle.loads(pickle.dumps(fig1))
    print(fig2.number)

    It generates the following output:

    1
    2
    

    One may wonder, if pickle works, why deepcopy does not. (I don't know, but that doesn't mean there cannot be a good reason, obviously. I'm just saying I'm a bit confused.)
13. QuLogic commented on Nov 9, 2021

    

    QuLogic

    on Nov 9, 2021

    Member

    More actions

    I don't understand what you mean here. With the attached PR, the code in the OP works on both Python 3.7 and 3.10.
14. greglucas commented on Nov 9, 2021

    

    greglucas

    on Nov 9, 2021

    Contributor

    More actions

    Oops, I think that was my bad in the comment on your PR! This indeed allows deepcopy to work without error, but it does not seem to "work" as expected, so I would say the NotImplementedError on deepcopy would still make sense here.

    Here, I get two figures, but the second one is missing some spines and the axes plot data.

    import matplotlib.pyplot as plt
    import copy
    fig, ax = plt.subplots()
    
    ax.plot([0, 1], [2, 3])
    
    fig2 = copy.deepcopy(fig)
    plt.show()
15. QuLogic commented on Nov 10, 2021

    

    QuLogic

    on Nov 10, 2021

    Member

    More actions

    Ah, I see yes.

    It's strange though that pickling seems to work since the copy docs say 'Classes can use the same interfaces to control copying that they use to control pickling.'
16. tacaswell commented on Nov 10, 2021

    

    tacaswell

    on Nov 10, 2021

    Member

    More actions

    I suspect that at a conceptual level, you are very correct that if pickle/unpickle works than deepcopy should also work (and vis versa), but at the object protocol level, Python gives us different magic methods for the two cases (which makes sense, pickle is a serialization protocol for sending data structures to a different process over the wire, deepcopy is for making a copy within the same process. If you can easily imagine that there are more efficient ways of doing the in-process copy than fully serializing for the wire). I think the current issue is exposing Matplotlib gets the pickling code path correct (and we test it) but does not get the deepcopy codepath right (yet). Chasing through getting deepcopy to be correct (the semantics are a bit subtle) will take an indeterminate time, where has intercepting the call on Figure an raising NotImplemented will be very easy ;) .
17. anntzer

    closed this as completedin #21597on Feb 2, 2022
18. QuLogic

    added this to the v3.6.0 milestone on Feb 2, 2022