Skip to content

Conversation

@esc24
Copy link
Member

@esc24 esc24 commented Nov 5, 2012

When setting the extent of a plot with GeoAxes.set_extent() the CRS of the provided coords currently defaults to a geodetic version of self.projection rather than self.projection. I do not see the reason for this and in my opinion the behaviour is unexpected. I propose changing it so that if I set up an axes in projection X, any provided extent is assumed to be in that coordinate system unless I explicitly tell it otherwise through the crs argument.

@pelson
Copy link
Member

pelson commented Nov 5, 2012

This behaviour was intentional. If you want to set the limits of your map in native coordinates you can do all of that with the standard matplotlib set_xlim and set_ylim, or as you say, you can do ax.set_extent(extent, crs=ax.projection).

The reason I did this in the first-place is that, 9 times out of 10, you know the extent of the map that you want in Lat Long, not in the (often arbitrarily unit-ed) coordinate system of the "native" map.

As such, I would want to maintain the capability for users to easily set their extents in latitude/longitude. Pragmatically, ax.set_extent(extent, crs=ccrs.Geodetic()) isn't so bad I suppose.

On the other hand, I can see that set_extent behaves differently compared to all of the other plotting routines which work in the native coordinate system by default. It is this inconsistency that is the "unexpected" behaviour you are talking about? If so, I'd be very tempted to change the plotting routines to default to latitude longitude (PlateCarree in the absence of a universal Geodetic support) as their default coordinate system.

@esc24
Copy link
Member Author

esc24 commented Nov 5, 2012

@pelson - that is the unexpected behaviour I refer to. I actually like the pragmatic (as you put it) ax.set_extent(extent, crs=ccrs.Geodetic()). It make the code readable removing a hidden assumption which could be dangerous given the various interpretations of precisely what lat long means to different people. I also like the consistency of defaulting to the axes projection so that (conceptually at least) projections only take place if you explicitly provide a source crs.

@esc24
Copy link
Member Author

esc24 commented Nov 5, 2012

It's also worth noting the behaviour of get_extent() (despite what the docstring says) gives you the extent in the axes' projection if crs is None, not the geodetic version.

@esc24
Copy link
Member Author

esc24 commented Nov 6, 2012

Added a commit that fixes the docstring for get_extent

Copy link
Member

Choose a reason for hiding this comment

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

The sentence was a little awkward in the first place, perhaps:

If not crs is given, the returned extent will be in this projection's native coordinate system.?

@pelson
Copy link
Member

pelson commented Nov 9, 2012

I'm surprised there are no tests depending on this. Would you mind adding one to mpl/test_set_extent.py?

@esc24
Copy link
Member Author

esc24 commented Nov 9, 2012

Happy to. I have a growing collection of PRs that I need to add tests to after I've finished the feature artist work.

@pelson
Copy link
Member

pelson commented Nov 9, 2012

😄

@esc24 esc24 closed this Nov 16, 2012
@esc24 esc24 reopened this Nov 16, 2012
Copy link
Member

Choose a reason for hiding this comment

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

Because this is such a common requirement, I'm tempted to ask to to expose a context manager which has the following syntax:

with assert_warns(UserWarning):
    ax.get_extent()

Copy link
Member

Choose a reason for hiding this comment

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

@pelson
Copy link
Member

pelson commented Mar 14, 2013

@esc24 - there has clearly been a lot of good work put into this PR. We fundamentally disagree on one line in this whole PR (line 535 in geoaxes.py where crs = self.projection). I'm going to "pause" (close it and apply the "Paused" label) to this PR, but it would be great to get the rest of your excellent work in (just re-open when ready), and we can open a wider debate (perhaps on the mpl-devel mailing list) as to whether the default crs for set_extent should be Geodetic or self.projection.

Cheers,

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.

3 participants