Add orientation parameter to Boxplot and deprecate vert

[saranti]

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

• "closes #0000" is in the body of the PR description to link the related issue
• new and changed code is tested
• Plotting related features are demonstrated in an example
• New Features and API Changes are noted with a directive and release note
• Documentation complies with general and docstring guidelines

[oscargus]

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

[saranti]

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

[rcomer]

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

Edit: apparently not.

[saranti]

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

[timhoffm]

Modulo two documentation suggestions.

Thanks @saranti!

[rcomer]

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.

[rcomer]

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]

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]

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]

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

matplotlib/lib/matplotlib/__init__.py

Line 646 in 46b39ab

_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]

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]

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.