MNT: Change map_projection to projection for Axes creation #1980
Conversation
|
The change seems sensible. It's unclear to me whether the test failure is related. |
|
Hm... I am pretty sure it isn't related to this because this would have complained about bad "map_projection" or similar errors. Perhaps the numpy 1.22 floating point difference from AVX512... The difference is just over the tolerance now and only on the Linux runners. |
greglucas
force-pushed
the
geoaxes-projection-kw
branch
from
1a3170e
to
7c71977
Compare
|
@dopplershift, I think the failing tests were due to the floating point issues with AVX512 after all. I added a commit to increase the floating point dtype size 7c71977, which appears to have fixed the failing CI. I can break that out to another PR if it would help too. |
|
@dopplershift, I think the commit to address that other failed test example is in this PR... Maybe I should have broken it out :) |
greglucas
force-pushed
the
geoaxes-projection-kw
branch
2 times, most recently
from
3efcecb
to
98e92be
Compare
greglucas
force-pushed
the
geoaxes-projection-kw
branch
from
98e92be
to
59c5817
Compare
greglucas
force-pushed
the
geoaxes-projection-kw
branch
from
59c5817
to
dbf508a
Compare
| projection = kwargs.pop("map_projection") | ||
|
|
||
| # The :class:`cartopy.crs.Projection` of this GeoAxes. | ||
| if not isinstance(projection, ccrs.Projection): |
There was a problem hiding this comment.
So I missed this last time. If projection is left as its default of None, this check fails. Previously, it would have failed on kwargs.pop('map_projection'). Was there any reason you made it "optional"?
There was a problem hiding this comment.
Oh good catch. I can't say I gave this a ton of thought... I was more hoping for consistency in downstream calls like AxesGrid, rather than looking at the specifics.
I guess this should actually be required above. It was previously required as you say, but gave a different error if it wasn't included... I'm inclined to update the docstring to required and not change it to raising a KeyError.
There was a problem hiding this comment.
Fine with me. Just make sure to also remove the default to None.
There was a problem hiding this comment.
actually, I have no idea why I did that way in the first place... Let's just pop the new keyword in the else branch and then the same error will be thrown. But, we also have the additional check later on to see if it is a Projection.
The GeoAxes class now requires the projection keyword argument rather than map_projection to be more consistent with how a normal Axes/Subplot is created.
greglucas
force-pushed
the
geoaxes-projection-kw
branch
from
dbf508a
to
8000376
Compare
The GeoAxes class now requires the
projectionkeyword argument rather thanmap_projectionto be more consistent with how a normal Axes is created. In particular, this makes working with AxesGrid and other toolkits easier by enabling a consistent keyword when passingsubplot_kw,axes_class, etc...closes #1979