-
Notifications
You must be signed in to change notification settings - Fork 369
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
Provide projection-aware tick formatters. #401
Conversation
from cartopy.mpl.geoaxes import GeoAxes | ||
|
||
|
||
class _GeoFormatter(Formatter): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you decide to make this private? I can imagine users wanting to subclass it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I made the methods public but I forgot to do the same to the class...
@ajdawson - just wondered if you could comment on the existing Formatters ~ https://github.com/SciTools/cartopy/blob/master/lib/cartopy/mpl/gridliner.py#L83 which produce a similar result (to the test images). I've not had much of a chance to look at this code yet, so please forgive the naive question. |
Given how flaky image testing of text is with matplotlib, I'm keen to avoid testing these with visual tests. There is a method on a formatter ( |
These new formatters will transform the tick locations from native plot projection coordinates to These new formatters are formatter classes in their own right, as opposed to instances of |
I wanted to do this, but I found it tricky at first and ended up re-implementing much of the workings in the tests which is obviously not desirable. I think perhaps there is an opportunity to use mock axes objects to avoid some of this hassle (an axes object is required even for the |
I've added a new commit that revises the testing procedure and makes the base These commits will need squashing before eventual merge (just a reminder to myself). |
from cartopy.mpl.geoaxes import GeoAxes | ||
|
||
|
||
class GeoFormatter(Formatter): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ajdawson - just wondering if you have more of these subclasses up your sleeve? If not, I'm inclined to suggest this becomes PlateCarreeFormatter
- essentially I currently believe the GeoFormatter
is an attempt to generalise a problem which only needs two solutions (Latitude & Longitude) and as a result has interfaces which make the implementation more complex than they (currently) need to be (e.g. make_transform_args
and extract_transform_result
). Is that an unfair observation? I'm happy to accept this complexity if there are potentially good cases for which this interface is useful, but I think it is worth knowing what they are even if we don't implement them just yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd agree that currently this is just for Plate Carree and that I can't see a situation where it would be anything else. I decided that the added complexity (make_transform_args
and extract_transform_args
) were better than having repeated code in each of a latitude and longitude formatter, which do pretty much the same thing but in subtly different ways. That is in fact why I originally made this class private, it was not designed for other people to subclass, but simply to support common functionality in LatitudeFormatter
and LongitudeFormatter
classes. In my original implementation I was going to make _make_transform_args
and _extract_transform_result
private methods but I thought that was not right since they are required to implement the (then private) _GeoFormatter
class.
This seems to be in need of a refactor, suggestions very welcome.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be in need of a refactor, suggestions very welcome.
[Mostly note to self:] Perhaps a private fuinction to handle the common stuff instead of a shared parent class? One which accepts a function that transforms a single value, rather than two functions that tell it what arguments to use and how to get the value.
I've added another commit. This simplifies the code somewhat, but it retains its previous structure. I've flip-flopped and made the shared parent class private again (renamed as Considering |
I've had a look and the refactor looks good. However I have an issue with the |
I'm just taking this behaviour from the
I can't see an easy way around this issue. There labels are only formatted as they are drawn so if you want to use the same formatter for 3 different plots (on different projections) you are asking the formatter to know which one is being drawn. This is not an obvious point, but it seems like it is asking too much of the formatter. Perhaps a note needs to go into the docstring to explain this issue. |
""" | ||
raise NotImplementedError("A subclass must implement this method.") | ||
|
||
def hemisphere(self, value, value_source_crs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason why this is public? It would make the docs a little odd.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is public in the derived classes because it is the thing people will likely modify (if they modify anything at all). Will this class show up in the docs if it is private?
I'm half tempted to not bother including these in the base class at all, since it is meant to be a private implementation detail...
I've made slight modifications to this again. The parent class is now totally private (think of it only as an implementation detail, users shouldn't know or care that it exists). I've also added a note to the docstrings to indicate the limitations of use. I don't necessarily think the way this is implemented is ideal, but it fills an important gap. Users are increasingly asking how to label their plots, particularly Plate Carree plots since these are very common and users expect it to "just work". |
# Round the transformed values to the nearest 0.1 degree for display | ||
# purposes (transforms can introduce minor rounding errors that make | ||
# the tick values look bad). | ||
projected_value = round(10 * projected_value) / 10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if a significant figure approach might be better here, I'm bothered about this when we zoom in a long way...
Would you mind putting a what's new entry in (that could also be the place for the example if you don't want to put something in the gallery/a documentation page for ticker) |
I have added (many) more commits which should bring this up to scratch. I think I've addressed all the previously raised points. The test failure is the problem fixed by #414 I think. |
.. plot:: examples/tick_labels.py | ||
:width: 300pt: | ||
|
||
.. plot:: examples/tick_labels.py |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason for doing this twice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No... spurious copy/paste I guess.
In much the same way that I don't think the built-in matplotlib tick functionality is the way the final solution is going to go, this does provide a stop-gap which makes some frequently needed functionality easier. As a result, I'd be prepared to merge this with a few tweaks. @esc24 - since you originally exposed the |
I'm 👍 on the approach. I just had a go with it and I like it. |
ccrs.Mercator)): | ||
raise TypeError("This formatter cannot be used with " | ||
"non-rectangular projections.") | ||
target = ccrs.PlateCarree() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd consider doing this in __init__
and assigning to self._target_projection
or similar to save instantiating a new object on every call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. I'll implement this and ping you back when done.
@ajdawson - A minor comment to consider and a fix to a conflict in what's new and I'll merge. |
Also added notes to docstrings about reuse of the formatter classes.
Tick locations are rounded after transformation to prevent spurious values from making the plot look bad. The precision to which they are rounded is increased, and can be overridden by the user.
@esc24 - I've made the target projection a class variable, it never changes so it seems like a good option. The merge conflict should be eliminated now too. |
Provide projection-aware tick formatters.
This PR adds projection-aware tick formatters for latitude and longitude axes. These fit with the current tick labelling capabilities, designed only for rectangular projections. They work by transforming the tick values to the
PlateCarree
projections before doing the formatting, resulting in meaningful tick values. The intention here is to provide something that "just works" for the most common cases (see #204), but future efforts may be better off going in the direction of improving the grid line labelling abilities of cartopy.