Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Supportax.grid(visible=<bool>).#18769
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Remove the separate _gridOnMajor/_gridOnMinor attributes and storevisibility together with all other grid attributes in the_major_tick_kw/_minor_tick_kw dicts under the "gridOn" key. This makesit possible to update just that entry in `Axis.grid`.Also, don't normalize `b` to True `if len(kwargs)` in `Axes.grid`:we need to keep it as `None`, normalizing it only to True/False in`Axis.grid` (which handles that just fine), so that `Axis.grid` canhandle the consistency checks between `b` and `visible`.
That issue was a regression from 3.2.2. Is this entire change necessary to fix it? I'm trying to determine whether this whole PR would need to go back, or some smaller version only. |
anntzer commentedOct 20, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I didn't know it was a regression, so I have no idea whether there's a simpler fix... |
| self.set_tick_params(which='minor', gridOn=self._gridOnMinor, | ||
| **gridkw) | ||
| if b is None and not forced_visibility: | ||
| gridkw['gridOn'] = not self._minor_tick_kw['gridOn'] |
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.
Should this toggle the value in_minor_tick_kw?
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.
set_tick_params (called just below) does that.
| """ | ||
| if len(kwargs): | ||
| if not b and b is not None: # something false-like but not None |
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.
Shouldn't we instead renameb tovisible?
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.
b=None (== toggle) andvisible=None (== False) behave differently. I think all these toggling APIs are pretty terrible, but breaking all the scripts in the world that doax.grid() to toggle the grid on is just not worth it.
I guess wecould aim to have the end API bedef grid(visible=True, ...): (and deprecateb) though. Then the only case we'd break is people doingax.grid() to switch off the grid when the grid is already on, which is probably much less common (andax.grid(False) just seems much clearer). But let's keep that discussion for another time.
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.
Uh, it's worse than I thought.
Owee, I'm MrMeeseeks, Look at me. There seem to be a conflict, please backport manually. Here are approximate instructions:
And apply the correct labels and milestones. Congratulation you did some good work ! Hopefully your backport PR will be tested by the continuous integration and merged soon! If these instruction are inaccurate, feel free tosuggest an improvement. |
…3.3.xBackport PR#18769 on branch v3.3.x
Dapid commentedNov 26, 2020
This change has broken other packages. Is it really necessary to remove _gridOnMajor/_gridOnMinor? |
Do we really have no public interface to check whether the grid is on? Downstream libraries are now replacing the private |
It would be helpful if other packages open an issue if they are stuck using a private attribute to access information they need. |
Remove the separate _gridOnMajor/_gridOnMinor attributes and store
visibility together with all other grid attributes in the
_major_tick_kw/_minor_tick_kw dicts under the "gridOn" key. This makes
it possible to update just that entry in
Axis.grid.Also, don't normalize
bto Trueif len(kwargs)inAxes.grid:we need to keep it as
None, normalizing it only to True/False inAxis.grid(which handles that just fine), so thatAxis.gridcanhandle the consistency checks between
bandvisible.Closes#18758.
PR Summary
PR Checklist
pytestpasses).flake8on changed files to check).flake8-docstringsandpydocstyle<4and runflake8 --docstring-convention=all).doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).