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

Update geoplot docs. #1044

Merged
merged 3 commits into from Jul 7, 2019
Merged

Conversation

ResidentMario
Copy link
Contributor

@ResidentMario ResidentMario commented Jul 7, 2019

Some minor renovations to the geoplot example gallery in the documentation necessitated by the geoplot@0.3.0 release. Also cleans up some code here or there.

Copy link
Member

@martinfleis martinfleis left a comment

Choose a reason for hiding this comment

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

Thank you for the update. I have only a few minor comments, mostly on consistency of docs.
Generally we use import geopandas throughout or docs as it is the most straightforward. Same might apply for import geoplot and submodules (if it is not something you want to specifically use as you did).

examples/plotting_with_geoplot.py Outdated Show resolved Hide resolved
examples/plotting_with_geoplot.py Outdated Show resolved Hide resolved
examples/plotting_with_geoplot.py Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jul 7, 2019

Codecov Report

Merging #1044 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1044   +/-   ##
======================================
  Coverage    89.7%   89.7%           
======================================
  Files          19      19           
  Lines        1836    1836           
======================================
  Hits         1647    1647           
  Misses        189     189

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 39385a1...0baed88. Read the comment docs.

@codecov
Copy link

codecov bot commented Jul 7, 2019

Codecov Report

Merging #1044 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1044   +/-   ##
======================================
  Coverage    89.7%   89.7%           
======================================
  Files          19      19           
  Lines        1836    1836           
======================================
  Hits         1647    1647           
  Misses        189     189

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 39385a1...6dbe78d. Read the comment docs.

@martinfleis martinfleis merged commit 42bc526 into geopandas:master Jul 7, 2019
@martinfleis
Copy link
Member

Thank you for updates. Merged into master.

Copy link
Member

@jorisvandenbossche jorisvandenbossche left a comment

Choose a reason for hiding this comment

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

@ResidentMario thanks for the updates!

small comment, will update myself in a PR as there are some other doc fixes I need to do as well

geopandas.datasets.get_path('nyc_boroughs')
)
collisions = geopandas.read_file(
geopandas.datasets.get_path('nyc_injurious_collisions')
Copy link
Member

Choose a reason for hiding this comment

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

Should this be from geoplot instead of geopandas? (and same for the one above)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it should. 😅

I didn't rerun the script after my last change, so shame on me for failing to notice this bad copy-paste job. If you want to address this yourself that's good, otherwise I can do a quick fixer-upper PR myself.

Copy link
Member

Choose a reason for hiding this comment

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

And shame on me that I didn't notice either and merged.

Copy link
Member

Choose a reason for hiding this comment

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

No, no, don't blame yourself too much, just a small error :-)

Copy link
Member

Choose a reason for hiding this comment

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

Already done #1046

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

3 participants