Skip to content

Fix to global plots #150

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

Merged
merged 1 commit into from
Nov 10, 2021
Merged

Conversation

pabloitu
Copy link
Collaborator

@pabloitu pabloitu commented Nov 4, 2021

Fixes #144. Modified plot_spatial_dataset and plot_catalog to correctly handle Global plots. Updated plot_customizations example in the docs.

Type of change:

  • [*] Bug fix (non-breaking change which fixes an issue)
  • [*] Documentation update (this pull request adds no new code)

Checklist:

  • [* ] I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • [*] I have made corresponding changes to the documentation
  • My changes generate no new warnings ### New versions of cartopy raises deprecation warning in shapely

…ument to plot_basemap. Does not define an extent in case that set_global is True. Also if set_global, region_border is modified to False to avoid global forecast edges when central_longitude is passed in the projection.

Modified plot_customizations example to pass -179 as central_longitude. Values of 180 or -180 are not correctly handled by cartopy and pcolor functions fails to project appropiately.
@pabloitu pabloitu requested a review from wsavran November 4, 2021 00:21
@pabloitu pabloitu changed the title Fixes #144 Fix to global plots Nov 4, 2021
@pabloitu pabloitu marked this pull request as ready for review November 4, 2021 00:26
@pabloitu
Copy link
Collaborator Author

pabloitu commented Nov 4, 2021

build is failing because of obspy apparently

@wsavran
Copy link
Collaborator

wsavran commented Nov 4, 2021

interesting. i just merged 2 prs today and didn't run into this issue. all builds were successful. any idea why it might be failing?

@wsavran
Copy link
Collaborator

wsavran commented Nov 4, 2021

obspy/obspy#2651
obspy/obspy#2654

these issues were supposedly fixed in the latest obspy release. it has to do with numpy dependencies it looks like.

@wsavran
Copy link
Collaborator

wsavran commented Nov 4, 2021

https://github.com/SCECcode/pycsep/runs/4098858335?check_suite_focus=true here is an example of a successful build.

@pabloitu
Copy link
Collaborator Author

pabloitu commented Nov 4, 2021

https://github.com/SCECcode/pycsep/runs/4098858335?check_suite_focus=true here is an example of a successful build.

It failed too by now, like if there was some change in dependencies within those 3 hour difference in between Merges. I will try to check it, but should not be a problem of this PR, since the changes were minimum.

@pabloitu
Copy link
Collaborator Author

pabloitu commented Nov 4, 2021

obspy/obspy#2651 obspy/obspy#2654

these issues were supposedly fixed in the latest obspy release. it has to do with numpy dependencies it looks like.

Did not read this, but arrived to the exact same conclusion. It was fixed, but for some reason our CI its getting a different obs/setup.py, where the line from distutils.errors import DistutilsSetupError is causing the error. In the pypi release this was changed fixed as you mention.
If I repeat the CI steps on my local machine and using pip to install python dependencies it works well. But don't get why our CI is using a different method to install dependencies.

@wsavran
Copy link
Collaborator

wsavran commented Nov 4, 2021

yeah i dont think we are seeing the same issue. in the past we had to pin numpy to 1.18.1 to solve this issue. it almost seems like there was some type of issue with the ci runner itself. im trying again to see if the problem was transient. i dont feel comfortable merging this until the ci issues are resolved.

@pabloitu
Copy link
Collaborator Author

pabloitu commented Nov 4, 2021

agreed.

@wsavran
Copy link
Collaborator

wsavran commented Nov 4, 2021

see this issue pypa/setuptools#2849.

@codecov-commenter
Copy link

Codecov Report

Merging #150 (90cc2ec) into master (9e53c0a) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #150   +/-   ##
=======================================
  Coverage   56.48%   56.48%           
=======================================
  Files          19       19           
  Lines        3233     3233           
  Branches      479      479           
=======================================
  Hits         1826     1826           
  Misses       1288     1288           
  Partials      119      119           

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 9e53c0a...90cc2ec. Read the comment docs.

@wsavran
Copy link
Collaborator

wsavran commented Nov 5, 2021

@pabloitu pr looks good, will merge this after release of v.0.5.0. ill make a new release v0.5.1 with this change included.

@wsavran wsavran merged commit 9a011aa into SCECcode:master Nov 10, 2021
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.

Global elipse projections not compatible with central_longitude argument
3 participants