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

Improved the gridline interface. #238

Merged
merged 4 commits into from Mar 14, 2013

Conversation

Projects
None yet
3 participants
@pelson
Member

pelson commented Mar 11, 2013

This is an intermediate step to improving the overall functionality of Cartopy gridlines & is by no means a future-proof API.

Adds some limited documentation with an example covering many of the basic user requirements. Further work will go into gridlines in the near future (either for v0.7 or v0.8).

Improved the gridline interface. This is an intermediate step to impr…
…oving the overall functionality of Cartopy gridlines & is by no means a future-proof API.

@ghost ghost assigned rhattersley Mar 12, 2013

@rhattersley

This comment has been minimized.

Show comment
Hide comment
@rhattersley

rhattersley Mar 12, 2013

Member

Hi @pelson - I'm starting to have a look through. In the meantime, would you mind adding a new commit which fixes the PEP 8 issues.

Member

rhattersley commented Mar 12, 2013

Hi @pelson - I'm starting to have a look through. In the meantime, would you mind adding a new commit which fixes the PEP 8 issues.

@pelson

This comment has been minimized.

Show comment
Hide comment
@pelson

pelson Mar 12, 2013

Member

Thanks for the nudge @rhattersley - all fixed by autopep8.

Member

pelson commented Mar 12, 2013

Thanks for the nudge @rhattersley - all fixed by autopep8.

import cartopy.crs as ccrs
from cartopy.tests.mpl import ImageTesting
from cartopy.mpl.gridliner import LATITUDE_FORMATTER, LONGITUDE_FORMATTER
@ImageTesting(['gridliner1'])

This comment has been minimized.

@rhattersley

rhattersley Mar 12, 2013

Member

The desired_gridline_prj and projections variables defined in the following couple of lines are unused.

@rhattersley

rhattersley Mar 12, 2013

Member

The desired_gridline_prj and projections variables defined in the following couple of lines are unused.

This comment has been minimized.

@pelson

pelson Mar 12, 2013

Member

Done.

@pelson

pelson Mar 12, 2013

Member

Done.

@rhattersley

This comment has been minimized.

Show comment
Hide comment
@rhattersley

rhattersley Mar 12, 2013

Member

The relevant copyright periods need updating.

Member

rhattersley commented Mar 12, 2013

The relevant copyright periods need updating.

#: A formatter which turns longitude values into nice longitudes such as 45S
LATITUDE_FORMATTER = mticker.FuncFormatter(lambda v, pos:
_north_south_formatted(v))
class Gridliner(object):
# NOTE: In future, one of these objects will be add-able to a GeoAxes (and

This comment has been minimized.

@rhattersley

rhattersley Mar 12, 2013

Member

Out of date reference to do_gridlines.

@rhattersley

rhattersley Mar 12, 2013

Member

Out of date reference to do_gridlines.

This comment has been minimized.

@pelson

pelson Mar 12, 2013

Member

Done.

@pelson

pelson Mar 12, 2013

Member

Done.

@@ -57,25 +123,73 @@ def __init__(self, axes, crs, draw_labels=False, collection_kwargs=None):
"""

This comment has been minimized.

@rhattersley

rhattersley Mar 12, 2013

Member

This docstring doesn't appear anywhere, so the constructor arguments aren't documented.

@rhattersley

rhattersley Mar 12, 2013

Member

This docstring doesn't appear anywhere, so the constructor arguments aren't documented.

This comment has been minimized.

@pelson

pelson Mar 12, 2013

Member

Fixed by change to conf.py. The change makes more docs available for GeoAxes too (but does not change any other class' documentation)

@pelson

pelson Mar 12, 2013

Member

Fixed by change to conf.py. The change makes more docs available for GeoAxes too (but does not change any other class' documentation)

# Tickers may round *up* the desired range to something tidy, not
# all of which is necessarily visible. We must be stricter with
# our texts, as they are drawn *without clipping*.
x_label_points = [x for x in x_ticks if x_lim[0] <= x <= x_lim[1]]

This comment has been minimized.

@rhattersley

rhattersley Mar 12, 2013

Member

Can we use the numpy.diff(x_lim) == 2 * crs._half_width check used earlier to chop off the last entry from x_label_points? It would seem to fix the "Known bug" in the labels test with the overlapping labels.

@rhattersley

rhattersley Mar 12, 2013

Member

Can we use the numpy.diff(x_lim) == 2 * crs._half_width check used earlier to chop off the last entry from x_label_points? It would seem to fix the "Known bug" in the labels test with the overlapping labels.

This comment has been minimized.

@pelson

pelson Mar 12, 2013

Member

I'm not sure - changing the central longitude sounds like that could be flaky. I'd like to keep this separate from this PR if that's ok?

@pelson

pelson Mar 12, 2013

Member

I'm not sure - changing the central longitude sounds like that could be flaky. I'd like to keep this separate from this PR if that's ok?

'supported.'.format(prj=self.axes.projection))
return True
def _axes_domain(self, nx=None, ny=None, background_patch=None):

This comment has been minimized.

@rhattersley

rhattersley Mar 12, 2013

Member

Currently the first and last labels are not included, because of the 1e-9 fudge factor used in this routine, and numerical precision issues in the data transform. But if we add a pass-through in this method for self.crs == self.axes.projection then it'd be possible to get the first and last labels to appear using something like:

ax.set_xlim((137, 143))
gl = ax.gridlines(draw_labels=True)
gl.xlocator = mticker.FixedLocator(range(137, 144))

Either way - it'd be nice to have a test that shows this.

NB. With a smarter version of ax.set_extent it might be possible to use it instead of ax.set_xlim but I think that should be kept out of this PR.

@rhattersley

rhattersley Mar 12, 2013

Member

Currently the first and last labels are not included, because of the 1e-9 fudge factor used in this routine, and numerical precision issues in the data transform. But if we add a pass-through in this method for self.crs == self.axes.projection then it'd be possible to get the first and last labels to appear using something like:

ax.set_xlim((137, 143))
gl = ax.gridlines(draw_labels=True)
gl.xlocator = mticker.FixedLocator(range(137, 144))

Either way - it'd be nice to have a test that shows this.

NB. With a smarter version of ax.set_extent it might be possible to use it instead of ax.set_xlim but I think that should be kept out of this PR.

This comment has been minimized.

@pelson

pelson Mar 12, 2013

Member

I don't disagree with your conclusions on this, but I'm a little weary of taking this on in this PR. Are you happy for me to extract this one out?

@pelson

pelson Mar 12, 2013

Member

I don't disagree with your conclusions on this, but I'm a little weary of taking this on in this PR. Are you happy for me to extract this one out?

@rhattersley

This comment has been minimized.

Show comment
Hide comment
@rhattersley

rhattersley Mar 12, 2013

Member

Either way - it'd be nice to have a test that shows this.

Or perhaps a less contrived example to support the existing one.

Member

rhattersley commented Mar 12, 2013

Either way - it'd be nice to have a test that shows this.

Or perhaps a less contrived example to support the existing one.

@rhattersley

This comment has been minimized.

Show comment
Hide comment
@rhattersley

rhattersley Mar 12, 2013

Member

OK @pelson - I'm done for now. A few minor issues/questions, but basically a very useful step forwards, thanks. :-)

Member

rhattersley commented Mar 12, 2013

OK @pelson - I'm done for now. A few minor issues/questions, but basically a very useful step forwards, thanks. :-)

@pelson

This comment has been minimized.

Show comment
Hide comment
@pelson

pelson Mar 12, 2013

Member

Notes:

Related to the comment about the constructor docstring ... perhaps this (and axes and collection_kwargs) should have a sphinx comment.

Not done. I'm not too fussed about this. I see the whole interface as a stop-gap which I can't see lasting longer than 1 or 2 versions. Generally, a user shouldn't be accessing the axes or crs and in many ways, I'd be tempted to make them private. FWIW the __init__ docs do now appear.

Or perhaps a less contrived example to support the existing one.

Not done. I've put the term "contrived" in the comment, but I did want to demonstrate as much functionality in a short-ish example.

Currently the first and last labels are not included, because of the 1e-9 fudge factor used in this routine, and numerical precision issues in the data transform. ...

Not done. I'd like to pull this out of this PR if that is a-ok.

Can we use the numpy.diff(x_lim) == 2 * crs._half_width check used earlier to chop off the last entry from x_label_points?

Ditto. I'm not confident this solution will work, but I'm happy to give it a shot outside of this PR.

Thanks @rhattersley!

Member

pelson commented Mar 12, 2013

Notes:

Related to the comment about the constructor docstring ... perhaps this (and axes and collection_kwargs) should have a sphinx comment.

Not done. I'm not too fussed about this. I see the whole interface as a stop-gap which I can't see lasting longer than 1 or 2 versions. Generally, a user shouldn't be accessing the axes or crs and in many ways, I'd be tempted to make them private. FWIW the __init__ docs do now appear.

Or perhaps a less contrived example to support the existing one.

Not done. I've put the term "contrived" in the comment, but I did want to demonstrate as much functionality in a short-ish example.

Currently the first and last labels are not included, because of the 1e-9 fudge factor used in this routine, and numerical precision issues in the data transform. ...

Not done. I'd like to pull this out of this PR if that is a-ok.

Can we use the numpy.diff(x_lim) == 2 * crs._half_width check used earlier to chop off the last entry from x_label_points?

Ditto. I'm not confident this solution will work, but I'm happy to give it a shot outside of this PR.

Thanks @rhattersley!

@rhattersley

This comment has been minimized.

Show comment
Hide comment
@rhattersley

rhattersley Mar 12, 2013

Member

I'm OK with that summary. (For the docstring issue I was only after an either-or solution.) But it would be useful to try the two open questions in a new PR.

More importantly, the tests aren't passing thanks to some minor text shuffling.

Member

rhattersley commented Mar 12, 2013

I'm OK with that summary. (For the docstring issue I was only after an either-or solution.) But it would be useful to try the two open questions in a new PR.

More importantly, the tests aren't passing thanks to some minor text shuffling.

@pelson

This comment has been minimized.

Show comment
Hide comment
@pelson

pelson Mar 12, 2013

Member

More importantly, the tests aren't passing thanks to some minor text shuffling.

Yes, though "minor" is not what I would call it. With an tolerance of 36 (which is what it is on travis) I was able to completely remove the right hand side ticks of the middle-right plot and still have the test passing. The new image comparison coming in with mpl 1.3 will help, but it does not resolve the major problems with text comparison using matplotlib. I'm confident there is some work that can be done to improve this situation in matplotlib...

Alas, for now, a tolerance of 35 will have to suffice.

Member

pelson commented Mar 12, 2013

More importantly, the tests aren't passing thanks to some minor text shuffling.

Yes, though "minor" is not what I would call it. With an tolerance of 36 (which is what it is on travis) I was able to completely remove the right hand side ticks of the middle-right plot and still have the test passing. The new image comparison coming in with mpl 1.3 will help, but it does not resolve the major problems with text comparison using matplotlib. I'm confident there is some work that can be done to improve this situation in matplotlib...

Alas, for now, a tolerance of 35 will have to suffice.

@rhattersley

This comment has been minimized.

Show comment
Hide comment
@rhattersley

rhattersley Mar 12, 2013

Member
More importantly, the tests aren't passing thanks to some minor text shuffling.

Yes, though "minor" is not what I would call it.

I'd call it minor, but MPL's image comparison calls it major. So who's right and who's wrong. ;-)

Member

rhattersley commented Mar 12, 2013

More importantly, the tests aren't passing thanks to some minor text shuffling.

Yes, though "minor" is not what I would call it.

I'd call it minor, but MPL's image comparison calls it major. So who's right and who's wrong. ;-)

@rhattersley

This comment has been minimized.

Show comment
Hide comment
@rhattersley

rhattersley Mar 14, 2013

Member

So, with the changes up to 80c0529 this is OK to go in. Is it worth a squash?

(PS. I'm a bit out of the loop at the moment, so if you want to get someone else to merge this that's fine by me.)

Member

rhattersley commented Mar 14, 2013

So, with the changes up to 80c0529 this is OK to go in. Is it worth a squash?

(PS. I'm a bit out of the loop at the moment, so if you want to get someone else to merge this that's fine by me.)

@pelson

This comment has been minimized.

Show comment
Hide comment
@pelson

pelson Mar 14, 2013

Member

Is it worth a squash?

I'm not too worried by them, they are logically reasonable and I think the commit messages are meaningful. I would be eager if there were many more (or the commit messages were meaningless).

Member

pelson commented Mar 14, 2013

Is it worth a squash?

I'm not too worried by them, they are logically reasonable and I think the commit messages are meaningful. I would be eager if there were many more (or the commit messages were meaningless).

esc24 added a commit that referenced this pull request Mar 14, 2013

Merge pull request #238 from pelson/tickmark_control
Improved the gridline interface.

@esc24 esc24 merged commit cbac465 into SciTools:master Mar 14, 2013

1 check passed

default The Travis build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment