Labels visibility from side spines (fix #1530)

[stefraynaud]

Rationale

It now takes into account the side label visibility in the rotation of labels.

Implications

Closes #1530 #1541 #1559 #1560 #1576 #1642 #1666 #1722 #1758

[dopplershift]

@QuLogic Does this fit with getting 0.18 out the door?

[QuLogic]

I think bug fixes should be okay.

[QuLogic]

This could use a test though; doesn't have to be an image test, just a check that the expected labels are set visible.

[stefraynaud]

I'll add a test tomorrow.

[QuLogic]

Do not include master! This is going into v0.18.x.

[QuLogic]

This cannot go into 0.18 with all the removals from master. I'm also not super keen on adding new functionality after the rc, especially without tests.

[stefraynaud]

@QuLogic I agree with you. This is more for 0.19.

[greglucas]

This seems quite complicated for a beginner to think about now with all the label options. A nice example showing all of these gridliner uses and why someone may be interested in them might be nice. Before it was just True/False, now it can be boolean, string, list, or dict in addition?

boolean: whether or not to draw labels
string of x/y: will only draw labels of the given CRS coordinate string, but draw them on any side?
list: will draw any x/y CRS labels, but only on the input sides?
dictionary: can define x/y for any side.

I also wonder if this may be adding too many options to do the same thing in multiple places. Do we want to limit the number of options for this at all?

There are really only 8 total options {x, y}{top, bottom, left, right} I think? So, maybe we should bring all of those attributes back from their deprecation that I did previously since you're implementing that capability here. I guess my preference would be to drop the dictionary and string options and then the list could have top_x or top_labels_x or something similar as input to decide what to draw (probably try to make it exactly the same as the attribute access?). I think being explicit with only a few options will be easier to explain and maintain.

[stefraynaud]

Okay. I just wrote example of use for better reasoning.

• draw_labels=True: I want to draw all labels everywhere.
• draw_labels="x": I want to draw only the longitudes but everywhere.
• draw_labels=["bottom", "left"]: I want draw all labels but only on the bottom and left sides.
• draw_labels={"bottom": "x", "left": "y"}: I want to draw the the longitudes at the bottom and the latitudes at the left.

In the incoming PR that fixes #1541, I had this support:

• • draw_labels=["x", "bottom", "left"]: I want draw only longitude labels and only on the bottom and left sides.

In all case, we must make sure that the user can control everything.

If aliases are used as you suggest, here is the list (whatever the format):
xbottom, xtop, xleft, xright, ybottom, ytop, yleft and yright.

The current gallery example that is dedicated to the gridliner can be extended make it to clearer to the users.

[stefraynaud]

I think it can be decided later in another PR what kind of value to allow for draw_labels, especially when it comes to string shortcuts.
We should be go forward to not block other PR and bug fixes (#1566, #1560, #1541, #1530).

[greglucas]

I'm still getting hung up on all of the various ways that one can request the labels to be drawn with these changes and just thought of a question. Why do we have draw_labels rather than xlabels and ylabels? We already have xformatter, xlocator, and x_inline. Why don't we add xlabels to that to signify whether to add the labels or not? It could accept boolean or list of side spines to use ['top', 'bottom', 'left', 'right']. I would even go so far as to suggest that xlabels=True would map to ['top', 'bottom'] rather than all of them which we currently have.

The name xlabels may not be the best due to matplotlib using that to signify a list of text entries to the tick locations. But, something similar like draw_xlabels just to signify the coordinate of interest? Basically what I'm getting at is reorganizing the initial logic from side spines to coordinate axes. Instead of (top, bottom, left, right)[x, y] why not (x, y)[top, bottom, left, right]?

[stefraynaud]

My contribution goal on this aspect is to make the user be able to decide the details, while keeping the current API.
I'm personnally ok with these propositions, once it's clear.
Changing code for handling new options is not a big job, so no problem for me.

However, we must be careful on a few points:

• If we break the API, this contribution will be added to the 1.0 milestone, except if backward compatible.
• We must not be confused by the x/y letters used for coordinates and the ones used for spines. We must keep in mind that there may have, for example, x coordinate labels on the y spines, or that some labels are even not on any of the x and y spines but on the geo spine only.

[greglucas]

I agree with your assessment of not breaking the backwards API, which is why I'm hesitant to add the dictionary functionality because it would make things even trickier to change in the future. Currently we only allow True/False for the Gridliner, so you're adding two more cases, a list and a dict as input, which just seems like too many ways to allow for input. (I am advocating to keep the list functionality below though)

For a gridliner, the x/y are always in reference to its own coordinate system and not the "axes" projection I thought? Isn't that why the ['top', 'bottom', 'left', 'right'] come in to distinguish the axes projection sides.

What I'd propose is to add draw_xlabels and draw_ylabels keyword arguments to the current function. Then we can even keep draw_labels for a version or two and put a deprecation warning in to explicitly use draw_xlabels/draw_ylabels instead, but still maintain what is currently done pretty easily in that case.

Another options is to not add in any new options in draw_labels, but rather take back the deprecation warnings of xlabels_top, etc... and add in the full suite of other combinations too. It feels a bit awkward to control via attributes of the gridliner object rather than in just a single initial command.

[stefraynaud]

What I'd propose is to add draw_xlabels and draw_ylabels keyword arguments to the current function. Then we can even keep draw_labels for a version or two and put a deprecation warning in to explicitly use draw_xlabels/draw_ylabels instead, but still maintain what is currently done pretty easily in that case.

That's probably the best solution because

• it clarifies settings,
• it is consistent with other x/y options,
• it is always better that tuning with attributes.

It may be interesting to ask the opinion of other people to confirm these choices before coding them. @dopplershift @QuLogic @lukelbd ?

[stefraynaud]

A few figures that were made with the last commit : https://drive.google.com/drive/folders/15Sz5buIrjPcEz1FwniLgBDYn6GCXvNV7?usp=sharing

This fixes #1530 #1606 and some others.

[stefraynaud]

Solves exactly: #1530 #1541 #1559 #1560 #1576 #1642

[greglucas]

When going through the images you posted I noticed a few things that may be worth looking into:

• The Geostationary and Nearside projection rotated labels (gridlines) are at an angle and not horizontal
• The RotatedPole labels are inside the projection even though inside=False
• inside=True, zoom=True the InterruptedGoode projection has a label that projects outside of the bounds
• Some of the labels on the top/bottom artists go opposite directions. On the Mercator image of zoom=True, rotate=gridlines the top label directions are 180 degrees flipped compared to the bottom labels.

There are a lot of lists of artists that you're carrying around here just to see which side/coordinate it belongs to. I'm wondering if you can abstract that information out to a LabelArtist() class that would contain the information of where it is, what side it belongs to, and be able to calculate intersections of itself and the boundary, etc... (Not saying you have to do this)

For your comment about uploading image comparison tests: I personally think we should get rid of the massive images with all of the projections and just choose a few of the representative projections to test. Every time a little update/position change occurs we either have to change the tolerance or update a 1 MB file in the repo and they are pretty fickle with alignments/positioning too. Maybe move the current test images to an Example instead?

[stefraynaud]

@greglucas here are a few answers.

Gridlines are not parallel in the GeoStationary and Nearside projections, and even less near the map boundary, as you can see on the following figure. A solution would be to take more points to estimate the gridline angle.

Ok for the opposite direction of labels. I'll try to find a solution.

Let see later for other problems.

[stefraynaud]

I think all labels that are inside the map boundary with inside=False are inline labels.
I should have switched them off..

[greglucas]

@stefraynaud, I apologize for losing track of this PR. Are there other changes you were going to make on this PR? If you can add any other changes and rebase I'll take a look at it again.

[stefraynaud]

Should I add an entry to the contributors?

[greglucas]

There are still a few documentation wordsmith suggestions I had, but other than that I think this is good to go.

[stefraynaud]

There are still a few documentation wordsmith suggestions I had, but other than that I think this is good to go.

I found the suggestions I missed. They are resolved now.

[adamgarbo]

Out of curiousity @greglucas, will this fix be pushed to the 0.19 release (e.g. 0.19.1) or included in 0.20?

Very much looking forward to these changes, which I believe will make a big difference in allowing Cartopy to produce publication-ready figures out of the box.

[greglucas]

This changes some API, so it will be v0.20.

@QuLogic, you have some changes requested on this. Mind taking another look?

This is going into a fresh 0.20 now. The block was for changes not to go into the 0.18 RC a while ago.

[greglucas]

Will give it a few more days here in case anyone wants to comment. I still think this is a significant improvement over our current state of things with the gridliner.

[stefraynaud]

Yes. Anyway, there will always be time to make improvements before the next release, just in case.

[greglucas]

@stefraynaud, thank you for your patience on this PR!! (and effort in keeping it moving)

[stefraynaud]

@greglucas Don't worry. I'm really happy that we took time to achieve this result. There are certainly things that can be improved, but I think we did things carefuly, taking all opinions into account, which was not easy. Good to colaborate with you!

[QuLogic]

Closes #1530 #1541 #1559 #1560 #1576 #1642 #1666 #1722 #1758

You need to give these all separate Closes, or it won't actually closed them all.

[QuLogic]

This test is broken; it doesn't compare res with result, and now (or maybe even since the beginning) res doesn't match what you've put as expected.

[lwk1542]

why does it be fixed in version 0.20？ the rotation of labels can not be set.
i set: gl.ylabel_style = {'rotation': 90}
but the angle of rotation is ~45