-
-
Notifications
You must be signed in to change notification settings - Fork 113
Add gridstyle #1680
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Draft
ahuang11
wants to merge
3
commits into
main
Choose a base branch
from
add_gridstyle
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Add gridstyle #1680
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -475,8 +475,11 @@ class HoloViewsConverter: | |
| fontsize : number or dict or None, default=None | ||
| Set title, label and legend text to the same fontsize. Finer control | ||
| by using a dict: ``{'title': '15pt', 'ylabel': '5px', 'ticks': 20}``. | ||
| grid : bool or None, default=None | ||
| Whether to show a grid. | ||
| grid : bool, str, or None, default=None | ||
| Whether to show a grid. If True, shows grid on both axes. | ||
| If ``'x'`` or ``'y'``, shows grid only on the specified axis. | ||
| Suffix with ``'dashed'``, ``'dotted'``, ``'dotdash'``, or ``'dashdot'`` | ||
| to change the grid line style, e.g. ``'x-dashed'``. | ||
|
|
||
| Resampling Options | ||
| ------------------ | ||
|
|
@@ -1059,7 +1062,16 @@ def __init__( | |
| plot_opts['logy'] = logy | ||
|
|
||
| if grid is not None: | ||
| plot_opts['show_grid'] = grid | ||
| if isinstance(grid, str): | ||
| gridstyle = {} | ||
| axis = grid[0] | ||
| other_axis = 'x' if axis == 'y' else 'y' | ||
| if len(grid) > 0: | ||
| line_dash = grid[1:].lstrip('-').lstrip('.').lstrip('_') | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why |
||
| gridstyle[f'{axis}grid_line_dash'] = line_dash | ||
| gridstyle[f'{other_axis}grid_line_alpha'] = 0 | ||
| plot_opts['gridstyle'] = gridstyle | ||
| plot_opts['show_grid'] = bool(grid) | ||
|
|
||
| if legend is not None: | ||
| plot_opts['show_legend'] = bool(legend) | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Are all these options supported by Matplotlib too? I recently looked into what
line_dash(bokeh) /linestyle(matplotlib) supported and documented it there https://hvplot.holoviz.org/en/docs/latest/ref/api/manual/hvplot.hvPlot.line.html#line-dash.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.
Thanks, I totally neglected matplotlib (and plotly); I'll see if there's a way to convert it; if not just the dash or dotted should be sufficient.
I personally think at that point, people should use
plot.opts(gridstyle={...}), else typing the{"line_dash": "dashed"}would be tedious. What do you think?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.
You can ignore Plotly.
Uh oh!
There was an error while loading. Please reload this page.
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.
Interestingly, holoviews mpl does not support gridstyle so I guess I need to implement it in hv first...
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.
Ugh :/ Had a discussion recently on Discord where I said HoloViews isn't a plotting library. That's exactly what I meant :D
I just realized something. When it comes to plot styling options, hvPlot usually acts as a pass-through. One advantage of that is that the keys and their allowed values are common between hvPlot and HoloViews (so it's easier for users to go from one to the other). One disadvantage is that the keys and their values are backend-dependent. As a result, we haven't attempted to build a "unified styling API" in hvPlot, that would work for all the supported backends.
It looks like this is where we are heading in this PR with e.g.
grid="x-dashed". Can I have your thoughts on that?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.
I actually just added a clause for dict support, so all of
grid='x','grid='x-dashed',grid={'grid_line_color': 'red'}works.To me, hvplot is not only a pass-through--(me being a PM for a second) it's a stress-free, direct plotting experience (at least for me), without having to deal with complex dict structures. Every time I have to use legend_opts or gridstyle, I have to pause and remember the valid key value pairs, which is why I like having a string like
x-dashedorx.dashedorxdashed.Uh oh!
There was an error while loading. Please reload this page.
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.
Oh and I also don't fully understand the backend_compat, so I dumped directly in the function, but I imagine there could be a better place if you can help me understand the flow of converter.py, maybe in
_process_style?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.
Yeah I'm not sure where to do that to be honest. I think you're pretty much introducing a new pattern with a unified plot API for multiple backends. Probably not
_process_styleasgridstyleis a plot option in HoloViews. But having that in the__init__.pydoesn't feel right either?