Skip to content

Add orientation parameter to Boxplot and deprecate vert#28074

Merged
rcomer merged 3 commits into
matplotlib:mainfrom
saranti:boxplot_vert
May 15, 2024
Merged

Add orientation parameter to Boxplot and deprecate vert#28074
rcomer merged 3 commits into
matplotlib:mainfrom
saranti:boxplot_vert

Conversation

@saranti

@saranti saranti commented Apr 14, 2024

Copy link
Copy Markdown
Contributor

PR summary

Closes #13435 together with PR #27998.
Replace the vert: bool parameter with orientation : {'vertical', 'horizontal'}. Changes are very similar to the work done on #27998 and most of it works interchangeably.

PR checklist

@saranti saranti added API: consistency Consistency of the matplotlib API, including naming, behavior, defaults, … Documentation: examples files in galleries/examples labels Apr 14, 2024
@oscargus oscargus added this to the v3.10.0 milestone Apr 15, 2024

@oscargus oscargus left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Will only leave comments for now, but in general looks OK once the tests are passing.

  1. Replace vert with orientation in the failing tests (you have added a similarity test for the transition period, so makes sense to update the old ones)
  2. Not sure how the discussion w.r.t. the rcParam went, but I think it makes sense to deprecate that as well. Not sure what the path is there though... boxplot.vertical

Comment thread doc/api/next_api_changes/deprecations/28074-TS.rst Outdated
Comment thread doc/users/next_whats_new/boxplot_orientation.rst Outdated
@saranti saranti marked this pull request as draft April 16, 2024 11:49
@saranti saranti marked this pull request as ready for review April 17, 2024 11:36
@saranti

saranti commented Apr 27, 2024

Copy link
Copy Markdown
Contributor Author

That test is passing on my end, I don't know what the problem is 🤔

@rcomer

rcomer commented Apr 27, 2024

Copy link
Copy Markdown
Member

Looks like the test just timed out. Let’s see if a re-spin helps…

Edit: apparently not.

Comment thread lib/matplotlib/axes/_axes.py
@saranti

saranti commented May 3, 2024

Copy link
Copy Markdown
Contributor Author
  • Now vert=True won't emit a deprecation warning, regardless of how it's set. It also won't override orientation.

  • vert=False will emit a deprecation warning and override orientation.

  • Setting mpl.rcParams['boxplot.vertical'] to False will trigger an rcparam deprecation warning.

Comment thread doc/api/next_api_changes/deprecations/28074-TS.rst Outdated
Comment thread lib/matplotlib/__init__.py Outdated
@saranti saranti force-pushed the boxplot_vert branch 3 times, most recently from bdcfc97 to 3bc3735 Compare May 7, 2024 09:19

@timhoffm timhoffm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Modulo two documentation suggestions.

Thanks @saranti!

Comment thread doc/api/next_api_changes/deprecations/28074-TS.rst Outdated
Comment thread doc/api/next_api_changes/deprecations/28074-TS.rst Outdated

@rcomer rcomer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @saranti, this is looking great!

It took me a while to get my head around why we are not warning when vert=True is explicitly passed. I guess it's because it's too complicated to do for bxp as we can't tell whether it came from the rcParam. However would it be worth adding an extra warning in boxplotspecifically for when vert=True? Apologies if I've missed some discussion on this somewhere.

Comment thread lib/matplotlib/tests/test_axes.py Outdated
np.random.lognormal(mean=1.25, sigma=1., size=(37, 4)), **stats_kwargs)
fig, ax = plt.subplots()
if bxp_kwargs.get('vert', True):
if not bxp_kwargs.get('orientation') or bxp_kwargs.get('orientation') == 'vertical':

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
if not bxp_kwargs.get('orientation') or bxp_kwargs.get('orientation') == 'vertical':
if bxp_kwargs.get('orientation', 'vertical') == 'vertical':

Defines a default and so avoids calling get twice.

@timhoffm

Copy link
Copy Markdown
Member

Thanks @saranti, this is looking great!

It took me a while to get my head around why we are not warning when vert=True is explicitly passed. I guess it's because it's too complicated to do for bxp as we can't tell whether it came from the rcParam. However would it be worth adding an extra warning in boxplotspecifically for when vert=True? Apologies if I've missed some discussion on this somewhere.

Good catch. Since we have the tri-state True/False/None, we can detect whenever True/False was passed explicitly. If we push rcParams resolution down to bxp() we can even detect that also for bxp().

@rcomer

rcomer commented May 12, 2024

Copy link
Copy Markdown
Member

If we push rcParams resolution down to bxp() we can even detect that also for bxp().

That would mean the rcParam affects cases where the user calls bxp directly, which it didn’t before. However I think it’s a little odd that it didn’t, so we could consider that a bugfix 🤔

@rcomer

rcomer commented May 12, 2024

Copy link
Copy Markdown
Member

Could we also set the rcParam in the default matplotlibrc to None and then add it to this dictionary (highlighted by @QuLogic above)?

_deprecated_remain_as_none = {}

If I have understood, that would mean we get the warning if the rcParam is explicitly set to either True or False by the user.

@timhoffm

timhoffm commented May 12, 2024

Copy link
Copy Markdown
Member

If we push rcParams resolution down to bxp() we can even detect that also for bxp().

That would mean the rcParam affects cases where the user calls bxp directly, which it didn’t before. However I think it’s a little odd that it didn’t, so we could consider that a bugfix 🤔

Indeed. But it would only affect bxp() calls, where vert is not passed and the rcParam is modified. I'd be willing to accept that change because being able to warn on all explicit bxp(..., vert=) is IMHO more important. And as you say, it can be considered a bugfix. And the rcParam gets deprecated, so the user has to adapt their code anyway.

Re rcParam None: I have not been able / spent enough time to fully understand the mechanics (see #28074 (comment)) but the solution there to warn on rcParam readout seems good enough. We only don't warn if you configure the rcParam explicitly to True (but why would one do that when it's the default behavior anyway).

@saranti

saranti commented May 14, 2024

Copy link
Copy Markdown
Contributor Author

I didn't see any reason why the vert = mpl.rcParams['boxplot.vertical'] needed to be in boxplot so I moved it to bxp. Now it also gives a warning to bxp calls where it previously didn't.
Explicitly setting vert=True will now also emit a deprecation warning.

@saranti saranti force-pushed the boxplot_vert branch 2 times, most recently from ca45848 to 0f7a522 Compare May 14, 2024 13:14
@rcomer rcomer merged commit 70a36e2 into matplotlib:main May 15, 2024
@saranti saranti deleted the boxplot_vert branch May 15, 2024 07:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

API: consistency Consistency of the matplotlib API, including naming, behavior, defaults, … Documentation: examples files in galleries/examples topic: pyplot API topic: styles

Projects

None yet

Development

Successfully merging this pull request may close these issues.

boxplot/violinplot orientation-setting API

5 participants