Skip to content

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

Merged
merged 1 commit into from
Feb 28, 2023

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Feb 26, 2023

PR Summary

Closes #25329.

PR Checklist

Documentation and Tests

  • Has pytest style unit tests (and pytest passes)
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • New plotting related features are documented with examples.

Release Notes

  • New features are marked with a .. versionadded:: directive in the docstring and documented in doc/users/next_whats_new/
  • API changes are marked with a .. versionchanged:: directive in the docstring and documented in doc/api/next_api_changes/
  • Release notes conform with instructions in next_whats_new/README.rst or next_api_changes/README.rst

@jklymak jklymak added the topic: pickle Issues to do with pickling label Feb 26, 2023
@@ -788,6 +788,19 @@ class Grouper:
def __init__(self, init=()):
self._mapping = {weakref.ref(x): [weakref.ref(x)] for x in init}
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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).

Copy link
Contributor Author

@anntzer anntzer Feb 27, 2023

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]

Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, done.

@greglucas greglucas merged commit 0489b7a into matplotlib:main Feb 28, 2023
@anntzer anntzer deleted the alg branch February 28, 2023 07:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: needs revision topic: pickle Issues to do with pickling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Unable to pickle figure with aligned labels
3 participants