-
-
Notifications
You must be signed in to change notification settings - Fork 396
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
[WIP]Add de-aliaser function for plots #1073
Conversation
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 would take advantage of the fact that alias_mapping
argument in cbook.normalize_kwargs
can be a subclass of Artist
objects.
Therefore, cbook.normalize_kwargs(plot_kwargs, mpl.lines.Line2D)
should do the trick and we can avoid maintaining the alias dicts. Regarding one or several functions, maybe we can wrap normalize_kwargs
for several common objects Line2D
(for plot
), PathCollection
(for scatter
) and so on.
Okay, I'd do the changes. We can use a single function for dealiasing. |
@OriolAbril This functionality is only present in unreleased latest version As a workaround I'd do something like this:
We can use this workaround until arviz shifts to new version of matplotlib. |
The workaround sounds good, in this case it is probably better to define a handful of dealiasers (or a dealiaser with some kind argument) to allow dealiasing for both plot and scatter and also hexbin of hist if they were to return a different artist than plot and scatter |
I have applied |
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.
Two comments only. I would name the function as something more explicit like matplotlib_kwarg_dealiaser
or normalize_matplotlib_kwargs
. Also, we have to make sure that after dealiasing, only the default prop name is used (e.g. no lw nor ms...)
I have checked the files and confirmed that only default prop names are used. Anyway, currently it won't cause any error and we can correct that alias aren't used in future . |
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.
How do you feel about adding a couple of tests for matplotlib_kwarg_dealiaser
?
Should I add more test cases or are these enough? |
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 think this test is enough. We are basically relying on matplotlib.
The dealiasing function is not working for python 3.5. Any workaround? |
matplotlib 3.1 droped support for python 3.5, so CI uses 3.0.3 which probably has different alias. Checking the keys and for which kinds it fails should allow to remove the combination from tests |
@percygautam I just had an idea. Currently most plots do something similar to:
We could simplify this by making Note: Also, to avoid merge issues with #1079 that does a considerable rewrite of plot_pair, it'd be better to not modify pairplot.py. |
I've done the changes and left |
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.
It looks like you have removed some blank lines in plot_pair docstring and now it the lint complains.
I have one question about matplotlib/bokeh interaction. What happens if there are keys in the kwargs dict that do not correspond to any alias nor accepted argument? is there any warning/error? Bokeh will have different names, so the dealiaser should not modify items that are not alias. Also, could you add a test to check for example that a dict like {"line_color": "black"}
goes through the dealiaser unchanged?
If the desired behaviour from above were not to happen (I don't really know how to check bokeh kwarg names though, sorry), we should also pass backend to the dealiaser so that None -> {}
, backend="bokeh" -> unchanged_input
and when none of the two conditions above are met, call matplotlib's normalize kwargs
Sorry for the delay. |
CHANGELOG.md
Outdated
* **Experimental Feature**: Added `arviz.wrappers` module to allow ArviZ to | ||
refit the models if necessary | ||
* **Experimental Feature**: Added `reloo` function to ArviZ | ||
* Added new helper function `matplotlib_kwarg_dealiaser` (#1073) | ||
refit the models if necessary (#771) | ||
* **Experimental Feature**: Added `reloo` function to ArviZ (#771) |
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.
There have been some issues with merging
kwargs.setdefault("linestyle", kwargs.pop("ls", "none")) | ||
kwargs.setdefault("linewidth", kwargs.pop("lw", _linewidth)) | ||
kwargs.setdefault("markersize", kwargs.pop("ms", _markersize)) | ||
kwargs.setdefault("linestyle", "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.
kwargs
should be dealiased before setting the defaults.
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 must have missed this one.
arviz/plots/pairplot.py
Outdated
.. plot:: | ||
:context: close-figs | ||
|
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 think these black blank lines are important for docs to render properly, could you check all inline example images are shown?
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.
Yes, there was some problem with blank lines. I checked all examples are working fine now.
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'll merge if tests pass |
Description
Fixes #1041 . Added function to dealiase the plot kwargs passed to plots by user. Working example can be found in cookbook.
Checklist