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

Cartopy 0.18.0 USCOUNTIES '20m' invalid #1368

Closed
ahuang11 opened this issue May 4, 2020 · 13 comments
Closed

Cartopy 0.18.0 USCOUNTIES '20m' invalid #1368

ahuang11 opened this issue May 4, 2020 · 13 comments
Labels
Area: Plots Pertains to producing plots Type: Bug Something is not working like it should
Milestone

Comments

@ahuang11
Copy link
Contributor

ahuang11 commented May 4, 2020

metpy/plots/cartopy_utils.py

USCOUNTIES = MetPyMapFeature('us_counties', '20m', facecolor='None', edgecolor='black')
ValueError: 20m is not a valid Natural Earth scale. Valid scales are "110m", "50m", and "10m".
@ahuang11 ahuang11 added the Type: Bug Something is not working like it should label May 4, 2020
@jthielen jthielen added the Area: Plots Pertains to producing plots label May 4, 2020
@jthielen jthielen added this to the 1.0 milestone May 4, 2020
@jthielen
Copy link
Collaborator

jthielen commented May 4, 2020

This looks to be the same issue that caused CI to fail in #1346.

@dopplershift I've tagged it as 1.0 for now, but would this warrant a 0.12.2 as well?

@dopplershift
Copy link
Member

Is this a full-out import failure? I haven't quite tried it out yet.

@jthielen
Copy link
Collaborator

jthielen commented May 5, 2020

Is this a full-out import failure? I haven't quite tried it out yet.

Based on when I tested it out quick, it is an import failure in metpy.plots, which further causes any of the projection helpers on the xarray accessors to fail.

I think this means that MetPy's MetPyMapFeature can no longer inherit from cartopy.feature.NaturalEarthFeature, since it really isn't a Natural Earth feature (in that it has differing scales of '500k', '5m', and '20m' instead of '10m', '50m', and '110m'), and instead needs to inherit directly from cartopy.feature.Feature and copy the other methods? Or can we monkey patch the new _validate_scale() method (although that seems risky given that it is private API)?

xref SciTools/cartopy#1506

@jthielen
Copy link
Collaborator

jthielen commented May 5, 2020

One complication I ran across when trying to implement a fix: Cartopy is LGPL licensed, so we can't directly copy/reuse any of its code in MetPy itself.

@jthielen
Copy link
Collaborator

jthielen commented May 5, 2020

Not sure if this is too hacky, but I was able to fix this (without copying any cartopy code) by modifying MetPyMapFeature.__init__() to

def __init__(self, name, scale, **kwargs):
    """Create USCountiesFeature instance."""
    super(cfeature.NaturalEarthFeature, self).__init__(ccrs.PlateCarree(), **kwargs)

    self.category = ''
    self.name = name
    self.scaler = (
        cfeature.Scaler(scale) if not isinstance(scale, cfeature.Scaler) else scale
    )

@dopplershift
Copy link
Member

sigh I hate everything. 😭 🤣

I'd be willing to put that "fix" in for 0.12.2. I think for master we should just implement something ourselves, relying on CartoPy's shapefile-reading functionality.

@AdamTheisen
Copy link

@dopplershift Not sure if it matters, but just wanted to +1 for the quick fix for 0.12.2. Just went to do a release on our software (act-atmos) but ran into the same error on conda-forge feedstock. I'd be happy to help out in any way.

@eliteuser26
Copy link
Contributor

Came across this issue recently when I upgraded both Metpy and Cartopy to the latest versions. It gave the error as mentioned in this issue. Put the fix which was suggested to remove the error from Cartopy. I would agree with the fix.

@cleebp
Copy link

cleebp commented Jun 8, 2020

Our team also ran into this bug after updating Cartopy, the fix above solved the issue.

@stefan-hofer
Copy link

stefan-hofer commented Jun 21, 2020

Also wanted to highlight that this issue breaks quite a large chunk of my metpy-related code. I'd be happy to help if help is needed.

@smarru
Copy link

smarru commented Jul 21, 2020

any updates on this issue? or mitigations on how to proceed forward?

@akrherz
Copy link
Contributor

akrherz commented Jul 21, 2020

@smarru if you are able to manually edit files in your python environment, just edit metpy/plots/cartopy_utils.py and change the 20m to 10m at the bottom on the file. That's all I did :/

@jthielen
Copy link
Collaborator

Fixed by #1416!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Plots Pertains to producing plots Type: Bug Something is not working like it should
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants