Skip to content

Add axes argument and return axes in plots #361

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Add axes argument and return axes in plots #361

wants to merge 7 commits into from

Conversation

chinandrew
Copy link
Collaborator

@chinandrew chinandrew commented Dec 4, 2020 •
edited
Loading

Closes #359

Summary of changes:

  • Add an optional axes argument so now you can use subplots more easily, e.g.
    fig, ax = plt.subplots(1,2)
    covidcast.plot(df2, ax = ax[0])
    covidcast.plot(df1, ax = ax[1])
  • Also have the functions return the axis instead of the figure since the axis may not be tied to one. There's another option to return fig, ax or just ax depending on the inputs, but not sure if its a good idea to have it return two different types. You can always get the figure with plt.gcf(), which is what the tests now do

@chinandrew
Copy link
Collaborator Author

oops need to fix some things for tests

@chinandrew
Copy link
Collaborator Author

oops need to fix some things for tests

Fixed.

@chinandrew chinandrew mentioned this pull request Dec 5, 2020
Open
capnrefsmmat
capnrefsmmat reviewed Dec 5, 2020
View reviewed changes
Copy link
Contributor

@capnrefsmmat capnrefsmmat left a comment

Choose a reason for hiding this comment

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

Overall looks good; I think just needs some doc tweaks

Python-packages/covidcast-py/covidcast/plotting.py Outdated
@@ -286,21 +289,22 @@ def _plot_bubble(ax: axes.Axes, data: gpd.GeoDataFrame, geo_type: str, **kwargs:
ax.legend(frameon=False, ncol=8, loc="lower center", bbox_to_anchor=(0.5, -0.1))


def _plot_background_states(figsize: tuple) -> tuple:
def _plot_background_states(figsize: tuple, ax: axes.Axes = None) -> axes.Axes:
"""Plot US states in light grey as the background for other plots.

:param figsize: Dimensions of plot.
Copy link
Contributor

Choose a reason for hiding this comment

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

So figsize is ignored if axes are provided? Should it also be optional?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added

Python-packages/covidcast-py/covidcast/plotting.py
@@ -79,7 +80,8 @@ def plot(data: pd.DataFrame,
Defaults to `True`.
:param kwargs: Optional keyword arguments passed to ``GeoDataFrame.plot()``.
:param plot_type: Type of plot to create. Either choropleth (default) or bubble map.
:return: Matplotlib figure object.
:param ax: Optional matplotlib axis to plot on.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the docstring should say what happens if ax is not provided (axes are made, states are plotted). Maybe that goes in the text above, such as in a paragraph after the one-line summary explaining that the maps are plotted on US states by default.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think I had a bug here, the background states are always plotted and I had left that statement out. I've also added a sentence describing the return behavior.

Python-packages/covidcast-py/covidcast/plotting.py


def plot_choropleth(data: pd.DataFrame,
time_value: date = None,
combine_megacounties: bool = True,
**kwargs: Any) -> figure.Figure:
**kwargs: Any) -> axes.Axes:
"""Plot choropleths for a signal. This method is deprecated and has been generalized to plot().
Copy link
Contributor

Choose a reason for hiding this comment

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

Want to use the deprecated directive here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added. This also made me realize our changelog is a bit incorrect, will make another PR to fix that.

@chinandrew
Copy link
Collaborator Author

running into some weird bugs with subplots, working through it now

@chinandrew
Copy link
Collaborator Author

:/
Screenshot from 2020-12-07 09-44-11

@chinandrew
Copy link
Collaborator Author

Fixed, was an axes limit issue

@dshemetov
Copy link
Collaborator

This looks like a backwards-compatible change. Should we fix and merge or close? @capnrefsmmat

@dshemetov dshemetov mentioned this pull request Jun 5, 2023
Open
9 tasks
@capnrefsmmat
Copy link
Contributor

Hmm, if this code still works, we might as well merge it. I don't think it's worth developing new features for the package in the long run, but since Andrew already did the hard work here, we might as well use it.

Are you able to test this branch and see if it still works? We'd have to fix the merge conflicts in the tests as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@capnrefsmmat capnrefsmmat capnrefsmmat left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add ax argument to plotting function
3 participants
@chinandrew @dshemetov @capnrefsmmat