-
Notifications
You must be signed in to change notification settings - Fork 362
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
FIX: Handle non-earth bodies within Projections #2283
Conversation
There were several places that initialized PlateCarree with no globe. We want to use the same globe as the Projection we are transforming between if the user doesn't pass one in explicitly.
lib/cartopy/mpl/ticker.py
Outdated
if source.globe != self._target_projection.globe: | ||
# The transforms need to use the same globe | ||
self._target_projection = ccrs.PlateCarree(globe=source.globe) |
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 is the only part that gives me pause. I'm not really sure what's going on here.
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.
That is because it is initialized as a class variable above without a globe:
cartopy/lib/cartopy/mpl/ticker.py
Line 23 in 0d58449
_target_projection = ccrs.PlateCarree() |
then we try to do the transform here in the call and it raises if the globes are different. But, we can't know what the source projection is until this point, and we don't want to create a new projection every call either I don't think, so I added this check for that case.
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.
Could we implement set_axis
(which exists from Formatter
->TickHelper
) to set what we need at that time?
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.
That is a good idea. I did that in a separate commit so it is easier to see the update. All of the tests we have currently call .axis = Mock(...)
, which avoids the set_axis()
path. So, do we think that anyone would call .axis = ...
on the formatter and thus we should keep the logic in the call method instead... Or I guess we could turn .axis
into a property?
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.
Given that .axis
and set_axis()
are upstream APIs, I think it makes the most sense for any notion of axis
being a property to be handled there.
I feel comfortable not worrying about users manually setting .axis
; my advice in that case would be for users to use set_axis
too.
Move the source and target projection calculation into the set_axis() call instead of the __call__ method.
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.
Other than the (now) need to update all those tests, this looks like a nice clean change.
There were several places that initialized PlateCarree with no globe. We want to use the same globe as the Projection we are transforming to/from if the user doesn't pass one in explicitly.
The OSGB test image had the gridlines shifted ever so slightly because that projection uses a different globe than the default PlateCarree (but still Earth radii, so no failures from PROJ). I think that we want to gridline/label to default to the same globe and this was incorrect before, but I'm also not 100% positive about that, so it'd be good for someone else to verify that logic. To avoid updating the test image I forced a
crs=PlateCarree()
with a default globe to be used within the gridliner to get the previous behavior.closes #2007