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

Properly document behavior of set_extent #1729

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kdpenner
Copy link
Contributor

@kdpenner kdpenner commented Feb 3, 2021

I'm not up-to-date on docstring conventions.

Rationale

Documentation for set_extent is unclear and wrong for one important case.

Implications

Clearer documentation!

Copy link
Contributor

@greglucas greglucas left a comment

Choose a reason for hiding this comment

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

I agree, this docstring could use a little more explanation. I'm a little confused by these examples though. I'm not clear why there are 3 cases here. There are only two, with/without crs given?

I think the most confusing part is assuming it to be a Geodetic version if no crs is given. So, if I've set up my axes to be Mercator(), I can't call ax.set_extent(ax.get_extent()), it raises an error for me! Maybe this should even be thought about more in the code, not just the docstring?

@kdpenner
Copy link
Contributor Author

kdpenner commented Feb 4, 2021

I'm a little confused by these examples though. I'm not clear why there are 3 cases here. There are only two, with/without crs given?

There's a carve out for a specific condition---a Plate Carree projection with no CRS given and global extent:

try_workaround = ((crs is None and
isinstance(self.projection, ccrs.PlateCarree)) or
crs == self.projection)
if try_workaround:
boundary = self.projection.boundary
if boundary.equals(domain_in_crs):
projected = boundary

The geodesic for [-180, 180, -90, 90] has no longitudinal extent. If Plate Carree with no CRS given behaved like every other projection, there should be no map. GeoAxes is smart but not smart enough:

>>> ax.set_extent([-180,180,-90,90], p.as_geodetic())
/Users/kpenner/miniconda3/envs/cartopy/lib/python3.7/site-packages/cartopy/mpl/geoaxes.py:834: UserWarning: Attempting to set identical left == right == 180.0 results in singular transformations; automatically expanding.
  self.set_xlim([x1, x2])
>>> ax.get_xlim()
(171.0, 189.0)
>>> ax.get_extent()
(171.0, 180.0, -90.0, 90.0)

Quite the gotcha.

I think the most confusing part is assuming it to be a Geodetic version if no crs is given. So, if I've set up my axes to be Mercator(), I can't call ax.set_extent(ax.get_extent()), it raises an error for me! Maybe this should even be thought about more in the code, not just the docstring?

Yes...this PR is my sneaky way of getting someone (perhaps me) to revisit #131.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

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