-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Support pickling of figures with aligned x/y labels. #25332
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
Conversation
@@ -788,6 +788,19 @@ class Grouper: | |||
def __init__(self, init=()): | |||
self._mapping = {weakref.ref(x): [weakref.ref(x)] for x in init} |
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 haven't looked at the Grouper much in detail, but would it be possible to use a weakkey dictionary here? Or a weakset for the values?
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 didn't check in detail, but weakref.py doesn't seem to make any specific allowance for pickling, so probably something like this PR would remain needed. I guess you could open a separate issue to track the usefulness of switching the implementation to weakkeydicts and weaksets, though.
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 agree you still need to go back and forth between weak and strong during pickling, but I'm a bit confused why you're doing it in Figure's getstate, rather than adding getstate/setstate to the Grouper class itself?
Here was a quick attempt at hacking it in with the WeakKeyDictionary, but now that I think about it, we might actually need the weakset as well here (I overlooked that there was a cycle from key: list(key)
in the key-value pairs)
main...greglucas:matplotlib:grouper-pickle
But I think that shows you could probably move your current implementation into getstate/setstate and it would be a little easier to follow all in one place (and allow other Grouper's to take advantage of it).
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.
Ah, yes. I think I was slightly confused because I also wrote the pickling of shared axes (via _AxesBase._shared_axes) and that one needs specific support (both because the Grouper is (effectively) a global living at the class level and thus doesn't get picked up by pickling, and also because even if it did you wouldn't want to pickle the entire Grouper (and thus all currently existing shared axes) but only the connected components of the axes currently being pickled). But here _align_label_groups is per-figure, so I guess just adding pickling support directly to Grouper is OK (and doesn't harm the above case.
Do you want to take over the issue and push your patch instead? :) [to be clear, the PR here is still correct AFAICT, but @greglucas' solution is more general]
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.
It will probably be a bit before I could get around to adding tests and thinking more about the proper weak-reference counting and refactoring that area properly, so I say we push forward here for now and leave that as a follow-up PR. Are you willing to move your weak/strong transitions over into Grouper's getstate/setstate instead of the classmethod and figure updates? I think this is good to go then.
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.
Sure, done.
PR Summary
Closes #25329.
PR Checklist
Documentation and Tests
pytest
passes)Release Notes
.. versionadded::
directive in the docstring and documented indoc/users/next_whats_new/
.. versionchanged::
directive in the docstring and documented indoc/api/next_api_changes/
next_whats_new/README.rst
ornext_api_changes/README.rst