-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
BUG: Warn when an existing layout manager changes to tight layout #24528
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
1ca4626
to
1133d85
Compare
1133d85
to
c029f1d
Compare
lib/matplotlib/tests/test_figure.py
Outdated
@@ -581,7 +582,8 @@ def test_invalid_layouts(): | |||
pc = ax.pcolormesh(np.random.randn(2, 2)) | |||
fig.colorbar(pc) | |||
with pytest.raises(RuntimeError, match='Colorbar layout of new layout'): | |||
fig.set_layout_engine("tight") | |||
with pytest.warns(UserWarning): | |||
fig.set_layout_engine("tight") |
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 find it odd that it both warns and then errors (the error means it doesn't change, and thus the warning is incorrect)
Consider moving the check up the call stack one level to set_layout_engine
such that only one of the error or warning occur.
c029f1d
to
4bbd822
Compare
lib/matplotlib/figure.py
Outdated
@@ -2576,6 +2576,10 @@ def set_layout_engine(self, layout=None, **kwargs): | |||
|
|||
if self._check_layout_engines_compat(self._layout_engine, | |||
new_layout_engine): | |||
if isinstance(self._layout_engine, ConstrainedLayoutEngine) \ |
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 think this is over-reach here. People are allowed to change layout engines.
The problem is if people have set the layout engine and then specifically call plt.tight_layout
, usually because they are copying and pasting magic commands, and then wonder why constrained layout got clobbered. This check should be in the tight_layout
method, not every time the user uses the API to switch the engine.
I'm not sure what @ksunden was referring to wrt to an error - it is very possible to call tight_layout without an error, so maybe the test was malformed.
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.
That makes sense, thank you.
Regarding the error, there was a possibility of a new UserWarning
immediately followed by a RuntimeError
from figure.set_layout_engine
.
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 was referring to a specific test that was included which had both a warning and an error with pytest checks for both. However in the event an error gets raised, the warning is pointless as that means the layout does not get changed.
I agree that I'm not sure it should warn quite so eagerly, but honestly not convinced it should warn for tight_layout
either... that seems like a pretty explicit "I want a tight layout" call, so not sure why we are looking to warn that it is doing exactly what is asked.
It feels like that is just as likely to warn users who expect it than it does to catch users who don't expect that behavior.
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.
but honestly not convinced it should warn for tight_layout either... that seems like a pretty explicit "I want a tight layout" call, so not sure why we are looking to warn that it is doing exactly what is asked.
If someone at the top of the code says plt.figure(layout='constrained')
and then at the bottom of the code says plt.tight_layout()
I think it is likely they have made a mistake.
4bbd822
to
061aad4
Compare
061aad4
to
94397f4
Compare
PR Summary
Warn the user with a
UserWarning
when a figure's layout changes from 'constrained' or 'compressed' to 'tight.'Addresses #24387
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