Skip to content
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

plot_by_id: pass kwargs to subplots and plot function #1258

Merged
merged 4 commits into from
Sep 6, 2018

Conversation

Dominik-Vogel
Copy link
Contributor

This pr enables the use of kwargs in plot_by_id. The kwargs applicable to plt.subplots are passed upon axis creation. The kwargs that remain are passed to the consequently called plotting function like plot, scatter or pcolormesh.

axes.append(ax)
else:
if len(subplots_kwargs) == 0:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this not be !=

@Dominik-Vogel
Copy link
Contributor Author

@jenshnielsen do you approve with the fixed comparison?

@codecov
Copy link

codecov bot commented Sep 6, 2018

Codecov Report

Merging #1258 into master will increase coverage by <.01%.
The diff coverage is 40%.

@@            Coverage Diff             @@
##           master    #1258      +/-   ##
==========================================
+ Coverage   70.66%   70.67%   +<.01%     
==========================================
  Files          74       74              
  Lines        8165     8174       +9     
==========================================
+ Hits         5770     5777       +7     
- Misses       2395     2397       +2

FIGURE_KWARGS.remove('kwargs')
SUBPLOTS_KWARGS = set(inspect.signature(plt.subplots).parameters.keys())
SUBPLOTS_KWARGS.remove('fig_kw')
SUBPLOTS_KWARGS = FIGURE_KWARGS.union(SUBPLOTS_KWARGS)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For readability i would prefer if the union was called something else that the individual set

Copy link
Collaborator

@jenshnielsen jenshnielsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Does it make sense to add a test for this?

@Dominik-Vogel
Copy link
Contributor Author

I decided against a test because there are not any tests for the plotting yet (except for the labels) and it is not highest prio....

@Dominik-Vogel Dominik-Vogel merged commit f63231a into microsoft:master Sep 6, 2018
giulioungaretti pushed a commit that referenced this pull request Sep 6, 2018
Merge: 631d956 0c777b4
Author: Dominik Vogel <30660470+Dominik-Vogel@users.noreply.github.com>

    Merge pull request #1258 from Dominik-Vogel/plot_by_id_kwargs
@Dominik-Vogel Dominik-Vogel deleted the plot_by_id_kwargs branch November 27, 2018 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants