Skip to content

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

Merged
merged 1 commit into from
Dec 7, 2022

Conversation

j1642
Copy link
Contributor

@j1642 j1642 commented Nov 21, 2022

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

  • 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

@oscargus oscargus added the topic: geometry manager LayoutEngine, Constrained layout, Tight layout label Nov 21, 2022
@j1642 j1642 force-pushed the warn-if-tight-layout branch from 1ca4626 to 1133d85 Compare November 21, 2022 15:36
@j1642 j1642 force-pushed the warn-if-tight-layout branch from 1133d85 to c029f1d Compare November 21, 2022 19:24
@@ -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")
Copy link
Member

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.

@j1642 j1642 force-pushed the warn-if-tight-layout branch from c029f1d to 4bbd822 Compare November 21, 2022 20:26
@@ -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) \
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Member

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.

@j1642 j1642 force-pushed the warn-if-tight-layout branch from 4bbd822 to 061aad4 Compare November 22, 2022 17:25
@tacaswell tacaswell added this to the v3.7.0 milestone Dec 6, 2022
@j1642 j1642 force-pushed the warn-if-tight-layout branch from 061aad4 to 94397f4 Compare December 6, 2022 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: geometry manager LayoutEngine, Constrained layout, Tight layout
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants