-
Notifications
You must be signed in to change notification settings - Fork 119
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
add legend as dict for line plots with test #71
Conversation
pyam/plotting.py
Outdated
Include a legend (`None` displays legend only if less than 13 entries) | ||
default: None | ||
legend : bool or dictionary, optional | ||
Add a legend. If a dictionary is provided, it will be used as keyword |
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.
W291 trailing whitespace
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 @gidden, I was thinking of doing something similar already!
lgtm, one suggestion for clearer docstring...
legend : bool or dictionary, optional | ||
Add a legend. If a dictionary is provided, it will be used as keyword | ||
arguments in creating the legend. | ||
default: None (displays legend only if less than 13 entries) |
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.
rephrase to 'as specified by MAX_LEGEND_LABELS
'?
Perhaps, if a user reads the docstring, they won't know what value
MAX_LEGEND_LABELS is. In the future, we should make that configurable (much
like matplotlib has rcparams), but I'm not sure that time is now..
…On Mon, Jul 16, 2018 at 11:44 AM, Daniel Huppmann ***@***.***> wrote:
***@***.**** commented on this pull request.
thanks @gidden <https://github.com/gidden>, I was thinking of doing
something similar already!
lgtm, one suggestion for clearer docstring...
------------------------------
In pyam/plotting.py
<#71 (comment)>:
> @@ -472,9 +475,10 @@ def line_plot(df, x='year', y='value', ax=None, legend=None, title=True,
The column to use for y-axis values
default: value
ax : matplotlib.Axes, optional
- legend : bool, optional
- Include a legend (`None` displays legend only if less than 13 entries)
- default: None
+ legend : bool or dictionary, optional
+ Add a legend. If a dictionary is provided, it will be used as keyword
+ arguments in creating the legend.
+ default: None (displays legend only if less than 13 entries)
rephrase to 'as specified by MAX_LEGEND_LABELS'?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#71 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABVAEfcMC_liGvda82GHBUYRxunnOQIYks5uHGB3gaJpZM4VK3GQ>
.
|
region plots now use _add_legend() add release notes appease stickler pep8 core.py
ok @danielhuppmann, this now implements a more intuitive way of setting our labels and I can now confirm that lineplot by composition works as I expect it to! Just tested with alphas. |
@danielhuppmann did you mean to add a commit here? |
great, thanks!
…On Tue, Jul 17, 2018 at 2:20 PM, Daniel Huppmann ***@***.***> wrote:
Merged #71 <#71>.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#71 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABVAEYK-Yx7wNgBWStcOvKpBZW6njIVDks5uHdaggaJpZM4VK3GQ>
.
|
Please confirm that this PR has done the following:
Adding to RELEASE_NOTES.md
Please add a single line in the release notes similar to the following: