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

Fix error on plot empty GeoDataFrame #571

Merged
merged 5 commits into from Oct 16, 2017

Conversation

Projects
None yet
3 participants
@darribas
Copy link
Contributor

commented Sep 29, 2017

This PR fixes #565 by introducing a check in plot_dataframe to make sure the table is not empty. If df.shape[0] > 0, it proceeds; otherwise, it issues a warning and returns the original ax (if nothing's passed, the None is returned).

@codecov

This comment has been minimized.

Copy link

commented Sep 29, 2017

Codecov Report

Merging #571 into master will increase coverage by 0.3%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #571     +/-   ##
=========================================
+ Coverage    94.4%   94.71%   +0.3%     
=========================================
  Files          14       14             
  Lines        1055     1059      +4     
=========================================
+ Hits          996     1003      +7     
+ Misses         59       56      -3
Impacted Files Coverage Δ
geopandas/plotting.py 93.81% <100%> (+0.19%) ⬆️
geopandas/tools/sjoin.py 94.82% <0%> (+1.84%) ⬆️
geopandas/tools/overlay.py 97.8% <0%> (+2.05%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fb075e8...72f0b8b. Read the comment docs.

@jorisvandenbossche

This comment has been minimized.

Copy link
Member

commented Sep 29, 2017

Thanks!

If you switch the if / else, I think you can prevent that you have to indent all the existing code (just to keep it a bit more readable), like

if not df.shape[0]:
    raise warning
    return ax

... current code

(probably you only need to move the checking of if ax is None a bit higher.

Further:

  • Can you also add a test for this ?
  • We should do the same for the series plot?
@darribas

This comment has been minimized.

Copy link
Contributor Author

commented Sep 29, 2017

Thanks @jorisvandenbossche Just pushed a fix that removes the indentation (good call!) and adds a similar approach to GeoSeries. I've just quickly checked and neither plot_dataframe nor plot_series get tested in geopandas/tests/. Where should I add the test how do you suggest I go about it? Thanks!

@jorisvandenbossche

This comment has been minimized.

Copy link
Member

commented Sep 29, 2017

There are plotting tests in geopandas/tests/test_plotting.py (they directly use the .plot method and not plot_dataframe, but have the same behaviour)

@jorisvandenbossche

This comment has been minimized.

Copy link
Member

commented Sep 29, 2017

And for the test itself, you can make an empty GeoDataFrame and GeoSeries, and try to plot it, and then make sure there is warning and no error. Checking the warning can be like https://docs.pytest.org/en/3.0.5/recwarn.html#warns

So something like:

s = GeoSeries([])
with pytest.warns(UserWarning):
    ax = s.plot()
assert len(ax.collections) == 0
warnings.warn("The GeoDataFrame you are attempting to plot is "
"empty. Nothing has been displayed.", UserWarning)
return ax

This comment has been minimized.

Copy link
@jorisvandenbossche

jorisvandenbossche Sep 29, 2017

Member

I would put this just below the lines below (the check if ax is None), so an empty plot is returned.

Although that is of course debatable which of the two behaviours is preferred (empty ax or None). But personally an empty ax seems more consistent and less unexpected (eg if you then add some labels to the ax after the plotting call). And is also what matplotlib does.

This comment has been minimized.

Copy link
@darribas

darribas Sep 29, 2017

Author Contributor

Good point! My thought was if you don't pass an ax, you should get nothing, but it makes sense that attempting to plot at least generates the axis object.

This comment has been minimized.

Copy link
@darribas

darribas Sep 29, 2017

Author Contributor

For plot_dataframe, the creation of ax is below, would it make sense to move it up for this reason?

This comment has been minimized.

Copy link
@jorisvandenbossche
@@ -260,6 +260,11 @@ def plot_series(s, cmap=None, color=None, ax=None, figsize=None, **style_kwds):
fig, ax = plt.subplots(figsize=figsize)
ax.set_aspect('equal')

if not s.shape[0]:

This comment has been minimized.

Copy link
@jdmcbr

jdmcbr Sep 30, 2017

Member

While this is not too difficult to understand, I think if s.empty: would make the logic clearer, and I'm not aware of any advantage of checking this way (though perhaps I'm missing something?). Same for the dataframe check below.

This comment has been minimized.

Copy link
@jorisvandenbossche

jorisvandenbossche Sep 30, 2017

Member

Yes, that could be a bit more readable.

@jorisvandenbossche jorisvandenbossche merged commit 87e7541 into geopandas:master Oct 16, 2017

4 checks passed

codecov/patch 100% of diff hit (target 94.4%)
Details
codecov/project 94.71% (+0.3%) compared to fb075e8
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jorisvandenbossche

This comment has been minimized.

Copy link
Member

commented Oct 16, 2017

@darribas Thanks!

jorisvandenbossche added a commit to jorisvandenbossche/geopandas that referenced this pull request Oct 17, 2017

@darribas

This comment has been minimized.

Copy link
Contributor Author

commented Oct 17, 2017

Thank you @jorisvandenbossche for the final touch!!! I'd forgotten this was empty!

jorisvandenbossche added a commit that referenced this pull request Oct 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.