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

Refactor interface to plot_geography #177

Merged
merged 4 commits into from
Nov 8, 2020
Merged

Conversation

dnerini
Copy link
Member

@dnerini dnerini commented Nov 1, 2020

Addressing FutureWarning from 1.3.2 which concerns the interface to the plot_geography method.

In short:

  • rename map to plot_map
  • set plot_map as optional argument

Remaining open question:
Should we remove option to use the deprecated basemap module and leave cartopy as unique option?

@dnerini dnerini added enhancement New feature or request discussion an open discussion for whole community labels Nov 1, 2020
@dnerini dnerini added this to the release v1.4 milestone Nov 1, 2020
@dnerini dnerini self-assigned this Nov 1, 2020
@codecov
Copy link

codecov bot commented Nov 1, 2020

Codecov Report

Merging #177 into master will increase coverage by 0.00%.
The diff coverage is 77.27%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #177   +/-   ##
=======================================
  Coverage   74.39%   74.40%           
=======================================
  Files         110      110           
  Lines        8815     8802   -13     
=======================================
- Hits         6558     6549    -9     
+ Misses       2257     2253    -4     
Impacted Files Coverage Δ
pysteps/visualization/animations.py 4.59% <ø> (+0.20%) ⬆️
pysteps/visualization/motionfields.py 72.65% <50.00%> (-1.23%) ⬇️
pysteps/visualization/precipfields.py 68.81% <66.66%> (-0.50%) ⬇️
pysteps/tests/test_plt_cartopy.py 84.21% <100.00%> (ø)
pysteps/tests/test_plt_motionfields.py 87.27% <100.00%> (ø)
pysteps/visualization/basemaps.py 49.42% <100.00%> (ø)

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 1dd9b63...1d7e9be. Read the comment docs.

@aperezhortal
Copy link
Member

aperezhortal commented Nov 5, 2020

Nice work! For me, it looks ready to merge.

Remaining open question:
Should we remove option to use the deprecated basemap module and leave cartopy as unique option?

I'm in favor of removing basemap since last August they made their (likely) final release and it won't be supported anymore.

@pulkkins
Copy link
Member

pulkkins commented Nov 6, 2020

Remaining open question:
Should we remove option to use the deprecated basemap module and leave cartopy as unique option?

For me it looks like there is no reason to use basemap anymore. In 0.18.0b1, they finally added a feature that was missing from cartopy. Namely, lon-lat labeling with different projections (SciTools/cartopy#1117).

@dnerini
Copy link
Member Author

dnerini commented Nov 8, 2020

All right, thanks @aperezhortal and @pulkkins. I'll merge this PR and open a new one for the deprecation of basemap.

@dnerini dnerini merged commit b88b1b2 into master Nov 8, 2020
@dnerini dnerini deleted the rename-keyword-plot_map branch November 8, 2020 09:27
@dnerini dnerini mentioned this pull request Nov 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion an open discussion for whole community enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants