Remove marginal plot grids as hardcoded default #1863
Conversation
|
The failing test seems unrelated to the changes I made. |
|
Can you post a screenshot of some figures with marginals and the default template, given these changes please? I’m not sure about “always wanted” btw... every explicit setting in PX is there for a reason ;) in this case I found the grid-less marginals with background color pretty ugly, and the bar heights harder to compare, as Weber’s Law would predict. |
|
Sorry, I didn't mean to imply that the current settings were there for no good reason and I agree that grid lines often facilitates comparisons. I just think the grid lines should be configurable and inherited from the template. So for the default template, there will still be grids in the marginal plots:
But with templates that don't include grids, the marginal plots will not add extra grids: |
|
Hmm I don’t love the loss of the continuity of grid lines from the main plot... We could leave the current PX settings alone if you set your new template’s grid lines to white tho! |
|
PS the new template looks very slick, looking forward to seeing it in 4.3! |
I considered that, but think it would be confusing if someone tried to add grids by setting
Except for histograms, plotly currently only adds gridlines on the y-axis for marginal plots, so although there is continuity from the main plot grid lines, there is still some visual dissonance when looking at a plot like the below since there are square grids only on 2 of 3 plots:
I see the value in the xy-grids for the histogram as both axes represent a value and not a grouping as in the rug plots. However, the default for histograms when created as standalone plots is to have only y-grid lines.
Although I they are much closer to the x-axis in the standalone plot, it is still easy to see the x-values for the histogram in the marginal plot since the grid lines from the main plot ends just below the histogram and there is little utility in continuing them. Just some aspects to consider before deciding.
Thank you! =D |
|
I think maybe a better fix would probably be to have PX check to see if the active template sets I doubt that trying to set this in the second/third plots is going to go well template-wise :) |
|
PX in general does need to tread more lightly to let templates show through, and I think this sort of "if the template doesn't set it, it's fair game, otherwise no" approach is a good approach... thoughts? |
|
I like it! I will try that tomorrow, first I am going to get a PR up for the template suggestions so you can have a look. |
|
@nicolaskruchten Added a commit with your suggested approach. |
| @@ -374,17 +374,27 @@ def configure_cartesian_marginal_axes(args, fig, orders): | |||
| set_cartesian_axis_opts(args, xaxis, "x", orders) | |||
|
|
|||
| # Configure axis ticks on marginal subplots | |||
| # Don't change the marginal grid settings if the template already specifies `showgrid` | |||
| if isinstance( | |||
| pio.templates[args["template"]].layout.xaxis.showgrid, bool | |||
There was a problem hiding this comment.
here you should check fig.layout.template actually
There was a problem hiding this comment.
Sorry, that's not right... you can just check args["template"] directly, at this point it should already be a template object.
There was a problem hiding this comment.
Sorry, that's not right either! but it should be... I'm going to PR that in.
There was a problem hiding this comment.
I removed the pio.templates part to be compatible with your upcoming PR. I also changed the comparison to be against None for clarity.
|
Thanks for pushing on this! I've rolled these changes into #1875 so I'll close this PR :) |
I suggest to remove that hardcoded grids for marignal plots and instead inherit the grid settings from the template. This is particularly useful for templates that don't use grids.
The second commit sets two defaults that I think are almost always wanted: No y-axis line or ticks for the marginal plots. I am happy to drop this change with the same logic as above if you think these should be configurable from templates as well.