FIX/API: do not reset backend key in rc_context#23299
FIX/API: do not reset backend key in rc_context#23299QuLogic merged 4 commits intomatplotlib:mainfrom
Conversation
|
I feel -0.5 on this. Do we really want to give users access to handling the backend state via a parameter to rc_context? That feels a bit out of place. Also, passing a user list of excludes is a foot gun because they can easily and unintendedly remove the backend exclusion. More generally, the need for special-handling backend here (and also in #23263) indicates to ne that we have some design issue here: I wonder whether we should track the backend state in rcParams at all. RcParams should only hold the defaults, not necessarily actual state. E.g. we store figure.figsize as a default, but the actual value is held by the figure instance. (That‘s not exactly comparable because we can have multiple figures but only one backend state. But say we could only have one figure, then we still would not store its size in rcParams. We shoul consider storing the actual backend in a central place outside of rcParams. |
👍 I don't think we should be storing state in rcParams. We probably could have a rule that nothing can set the rcParams inside the library so the user always has complete control over them. |
|
I also don't think we should expose this API. While I also agree that storing the backend somewhere else than in rcParams may have been better, this may be slightly tricky to change (in particular, you need to keep the ability to set the backend in the matplotlibrc file, and I guess it would be better not to break people using For the specific issue at hand, I think the problem is actually here: def __getitem__(self, key):
if key in _deprecated_map:
version, alt_key, alt_val, inverse_alt = _deprecated_map[key]
_api.warn_deprecated(
version, name=key, obj_type="rcparam", alternative=alt_key)
return inverse_alt(dict.__getitem__(self, alt_key))
elif key in _deprecated_ignore_map:
version, alt_key = _deprecated_ignore_map[key]
_api.warn_deprecated(
version, name=key, obj_type="rcparam", alternative=alt_key)
return dict.__getitem__(self, alt_key) if alt_key else None
# In theory, this should only ever be used after the global rcParams
# has been set up, but better be safe e.g. in presence of breakpoints.
elif key == "backend" and self is globals().get("rcParams"):
val = dict.__getitem__(self, key)
if val is rcsetup._auto_backend_sentinel:
from matplotlib import pyplot as plt
plt.switch_backend(rcsetup._auto_backend_sentinel)
return dict.__getitem__(self, key) def copy(self):
rccopy = RcParams()
for k in self: # Skip deprecations and revalidation.
dict.__setitem__(rccopy, k, dict.__getitem__(self, k))
return rccopy
|
|
From the API point of view we want to have
Every solution that fulfills this is a viable fix for #23298. Whether/how we could separate the backend state from |
|
Ah, I guess there's an important point that I missed here: direct assignment to |
The motivating issue is that if the backend was the auto backend sentinel and the backend was first fully resolved in an `rc_context` block, then we would reset back to the sentinel an exit which would lead to strange behavior with figures being erroneously closed. This special cases the 'backend' key to not be reset on `__exit__` Closes matplotlib#23298
97fcbd6 to
7fce03e
Compare
|
Changed to simply drop the backend key when making the internal copy going with Tim and Antony's suggestion. |
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
|
I mostly took your suggestion @timhoffm but re-worded it a little bit. |
QuLogic
left a comment
There was a problem hiding this comment.
These should not be linked:
/home/circleci/project/doc/api/next_api_changes/behavior/23299-TAC.rst:1: WARNING: py:obj reference target not found: rcParams['backend']
/home/circleci/project/doc/api/next_api_changes/behavior/23299-TAC.rst:4: WARNING: py:obj reference target not found: rcParams['backend']
/home/circleci/project/doc/api/next_api_changes/behavior/23299-TAC.rst:4: WARNING: py:obj reference target not found: matpotlib.rc_context
/home/circleci/project/doc/api/next_api_changes/behavior/23299-TAC.rst:4: WARNING: py:obj reference target not found: rcParams['backend']
ddc50d6 to
69babbd
Compare
69babbd to
545fff0
Compare
|
I went with the |
The motivating issue is that if the backend was the auto backend sentinel and
the backend was first fully resolved in an
rc_contextblock, then we wouldreset back to the sentinel an exit which would lead to strange behavior with
figures being erroneously closed.
Adding a general purpose exclusion list seems like a better option than specialcasing either the backend key or the backend value.
Special case
'backend'key to not be reset.Closes #23298
~If people like this idea I will write the tests and docs. ~
PR Summary
PR Checklist
Tests and Styling
pytestpasses).flake8-docstringsand runflake8 --docstring-convention=all).Documentation
doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).