Support individual styling of major and minor grid through rcParams #29481
Conversation
There was a problem hiding this comment.
Thank you for opening your first PR into Matplotlib!
If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks.
You can also join us on gitter for real-time discussion.
For details on testing, writing docs, and our review process, please see the developer guide
We strive to be a welcoming and open project. Please follow our Code of Conduct.
There was a problem hiding this comment.
Thanks for the PR. The solution is surprisingly simple. I would have thought that there are more complications because of the concurrent settings and the need for backward compatibility.
The only improvement needed is the clearer distinction on the color and linewidth parameter values between "none" and None.
lib/matplotlib/rcsetup.py
Outdated
| "grid.major.color": validate_color, # grid color | ||
| "grid.major.linestyle": _validate_linestyle, # solid |
There was a problem hiding this comment.
We have to be a bit more precise with these. The string "none" is a valid value for both. We should not repurpose it here with the meaning not defined. In the parsed rcParams dict, we need to be able to have both values None and "none".
Since matplotlibrc is purely text based, we have to distinguish by capitalization. 'None' must be parsed to None, and 'none' must be parsed to "none". Please create respective validate_color_or_None and validate_linestyle_or_None functions for that.
There was a problem hiding this comment.
Sounds good. I was thinking of doing something like that but I saw that the functions parse the "None" and "none", so I didn't know if I should introduce a new function or not. Nevertheless, this should be easy.
There was a problem hiding this comment.
Yes, it’s a bit unfortunate that the existing parameters are permissive wrt. to capitalization. But the new ones have to distinguish. We may deprecate accepting None for the other variants. But that’d be for a separate PR.
lib/matplotlib/axis.py
Outdated
| grid_linewidth = mpl._val_or_rc(grid_linewidth, "grid.linewidth") | ||
| grid_color = ( | ||
| mpl._val_or_rc(grid_color, "grid.color") | ||
| if mpl.rcParams[f"grid.{major_minor}.color"] == "none" |
There was a problem hiding this comment.
| if mpl.rcParams[f"grid.{major_minor}.color"] == "none" | |
| if mpl.rcParams[f"grid.{major_minor}.color"] is None |
see comment on validation
lib/matplotlib/axis.py
Outdated
| ) | ||
| grid_linestyle = ( | ||
| mpl._val_or_rc(grid_linestyle, "grid.linestyle") | ||
| if mpl.rcParams[f"grid.{major_minor}.linestyle"] == "none" |
There was a problem hiding this comment.
| if mpl.rcParams[f"grid.{major_minor}.linestyle"] == "none" | |
| if mpl.rcParams[f"grid.{major_minor}.linestyle"] is None |
see comment on validation
lib/matplotlib/axis.py
Outdated
| grid_alpha = mpl.rcParams["grid.alpha"] | ||
| grid_alpha = ( | ||
| mpl.rcParams["grid.alpha"] | ||
| if f"grid.{major_minor}.alpha" not in mpl.rcParams |
There was a problem hiding this comment.
Why is the logic here different (hasattr) compared to the other parameters (is None)?
There was a problem hiding this comment.
Honestly, I have no idea so the only explanation is stupidity. Furthermore, I think this is a bug because grid.minor/major.alpha is always in rcParams. I will fix this so that is the same with the rest.
|
One related aspect to think about: Do we anticipate that we'll need a distinction between x and y axis as well? Because if so, we should directly introduce these. |
The `*_or_None` validators have accepted any capitalization of the string "none" as input. This is overly permissive and will likely lead to conflicts in the future because we cannot distinguish between resolving to `None` and `"none"` which may be need for some parameters in the future. Inspired by matplotlib#29481.
|
I made the suggested changes.
Can you clarify what you meant by "a distinction between x and y axis"? Do you mean something like rcParams["grid.major.xaxis.linestyle"] = ":"
rcParams["grid.major.yaxis.linestyle"] = "-" |
The `*_or_None` validators have accepted any capitalization of the string "none" as input. This is overly permissive and will likely lead to conflicts in the future because we cannot distinguish between resolving to `None` and `"none"` which may be need for some parameters in the future. Inspired by matplotlib#29481.
|
Sorry I forgot to update the ".pyi" file |
Basically yes. Exact spelling t.b.d. |
lib/matplotlib/rcsetup.pyi
Outdated
| @@ -140,6 +141,7 @@ def validate_fillstylelist( | |||
| ) -> list[Literal["full", "left", "right", "bottom", "top", "none"]]: ... | |||
| def validate_markevery(s: Any) -> MarkEveryType: ... | |||
| def _validate_linestyle(s: Any) -> LineStyleType: ... | |||
| def _validate_linestyle_or_None(s: Any) -> LineStyleType | None: ... | |||
There was a problem hiding this comment.
Not sure why mypy fails for this function but it doesn't for the _validate_linestyle. The stub and runtime arguments are different for both.
There was a problem hiding this comment.
No idea. Just make the arguments the same.
I think this is possible however it might look "ugly". The change will require the One possible way is by accessing the Another way is to add an extra required argument to the constructor of Any thoughts on those? |
|
We already rely on matplotlib/lib/matplotlib/axis.py Lines 106 to 116 in 3b329d9 It's ok to use that. Spelling: We have "xaxis", "xticks", "xscale" as axis-dependent names (but they are all also used in the python interface). We do not have any xgrid or similar names. Options:
None of them look like the obvious right choice to me. |
|
Out of the four, I prefer two:
I will try to implement the second one for now. I can change the key later. |
The `*_or_None` validators have accepted any capitalization of the string "none" as input. This is overly permissive and will likely lead to conflicts in the future because we cannot distinguish between resolving to `None` and `"none"` which may be need for some parameters in the future. Inspired by matplotlib#29481.
|
I implemented the x and y axis distinction however I came across something that needs further clarification. Right now "grid.xaxis.major.linestyle" is a valid key but "grid.xaxis.linestyle" is not. Should it be a valid option as well? If so it is unclear which of "grid.major.linestyle"and "grid.xaxis.linestyle" should take precedence. |
|
No, we don't need "grid.xaxis.linestyle". If starting from scratch, I'd just have the granular settings "grid.xaxis.major.linestyle" etc. After all, these are advanced style configuration options, and if somebody cares to adapt them, it's ok to write out the details. We keep the high-level ones "grid.linestyle" for backward compatibility (and it is a nice convenience) but we don't need the middle-ground. |
There was a problem hiding this comment.
Looks good. Please add a What's New note according to https://matplotlib.org/devdocs/devel/api_changes.html#what-s-new-notes
`_val_or_rc` now accept multiple rc names and return val or the first non-None value in rcParams. Returns last rc name if all other are None. Also, simplified code in `Tick` for grid lines creatation
|
I added the What's New note. I am not if the syntax is correct for the rc parameters. I have never used rst before so let me know if it needs correcting. Also, I tried to resolve the stubtest unsuccessfully. |
I suppose you've tricked yourself. You've made the Try removing both allowlist entries. |
There was a problem hiding this comment.
Please also add a test. Basically, create a plot with the rcParams set and investigate the grid line properties, e.g. using ax.xaxis.get_gridlines()[0].get_color(). You might need same_color() for comparison if the returned color format does not match the input format.
| Using :rc:`grid.major.*` or :rc:`grid.minor.*` will overwrite the value in | ||
| :rc:`grid.*` for the major and minor gridlines, respectively. Similarly, | ||
| specifying :rc:`grid.xaxis.major.*` and :rc:`grid.yaxis.major.*` will overwrite | ||
| `grid.major.*` for x and y axis major gridlines respectively. |
There was a problem hiding this comment.
| `grid.major.*` for x and y axis major gridlines respectively. | |
| :rc:`grid.major.*` for x and y axis major gridlines respectively. |
Should fix sphinx.
| specifying :rc:`grid.xaxis.major.*` and :rc:`grid.yaxis.major.*` will overwrite | ||
| `grid.major.*` for x and y axis major gridlines respectively. | ||
|
|
||
| .. plot:: |
There was a problem hiding this comment.
The example is a bit heavy. This should be illustrative easy to grap. It should give an idea what you can do and how. It's not an exhaustive documentation of the feature. The rendered entry should fit on a (large) screen. Please boil down to one figure (you can have two Axes side by side if really needed) and take a subset of rcParams only.
Typically, one figure should be enough. Select a few parameters.
lib/matplotlib/rcsetup.py
Outdated
| "grid.xaxis.major.color": validate_color_or_None, # grid color | ||
| "grid.xaxis.major.linestyle": _validate_linestyle_or_None, # solid | ||
| "grid.xaxis.major.linewidth": validate_float_or_None, # in points | ||
| "grid.xaxis.major.alpha": validate_float_or_None, | ||
|
|
||
| "grid.xaxis.minor.color": validate_color_or_None, # grid color | ||
| "grid.xaxis.minor.linestyle": _validate_linestyle_or_None, # solid | ||
| "grid.xaxis.minor.linewidth": validate_float_or_None, # in points | ||
| "grid.xaxis.minor.alpha": validate_float_or_None, | ||
|
|
||
| "grid.yaxis.major.color": validate_color_or_None, # grid color | ||
| "grid.yaxis.major.linestyle": _validate_linestyle_or_None, # solid | ||
| "grid.yaxis.major.linewidth": validate_float_or_None, # in points | ||
| "grid.yaxis.major.alpha": validate_float_or_None, | ||
|
|
||
| "grid.yaxis.minor.color": validate_color_or_None, # grid color | ||
| "grid.yaxis.minor.linestyle": _validate_linestyle_or_None, # solid | ||
| "grid.yaxis.minor.linewidth": validate_float_or_None, # in points | ||
| "grid.yaxis.minor.alpha": validate_float_or_None, | ||
|
|
There was a problem hiding this comment.
Seeing this in code, it feels a bit too much. I've browsed though a lot of plots and didn't find relevant usage. If the grids are different on x and y, it's typically that only one of them is activated. I've not seen the case that people use different styles for x and y.
Therefore I revert my previous proposal and think we should leave this out (YAGNI). Sorry for the back-and-forth, but sometimes one has to go the step to see it was wrong. On the plus side, this interplay lead to the expansion of _val_or_rc() which I think is a real win.
There was a problem hiding this comment.
That is far. I will revert the changes. It's simple anyway with the new _val_or_rc() to go back-and-forth.
|
Do you want me to create the test in the file "test_rcparams.py" or create a separate file? |
|
Please put it in |
|
I was working on the test and I realised that the |
|
Then use |
|
I fix the spelling so all tests should pass now. If there is anything else please let me know. |
|
@timhoffm Any idea when this might be merged? |
|
This needs a second review by a core developer, and review time is scarce. However, this is flagged for 3.11 and I'm pretty sure it will get the attention in time for the next release. |
PR summary
Possibly closes #13919 issue. Additionally, PR #26121 also tried to solve the same issue but I understand that it is not backwards compatible and there haven't been any updates since 2023. My implementation should be backwards compatible.
I have tested the code with pytest. Some tests failed on my machine, but all were latex tests. The problem is probably my latex installation.
I also tested to code with a small python script that:
The results were identical and as expected.
Test
My test script:
Results
PR checklist