Skip to content
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

Deprecate old lat/lon tick label formatters #1066

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lbdreyer
Copy link
Member

@lbdreyer lbdreyer commented May 1, 2018

We have two (2) longitude and latitude tick formatters.

cartopy.mpl.gridliner.LONGITUDE_FORMATTER which is ~5 years old
and
cartopy.mpl.ticker.LongitudeFormatter which is ~ 4 years old

The newer one is better as it allows the user to modify the number format, etc.
I'm not sure why the old one wasn't removed when the new one was added, the code is so similar!

I think we should deprecate the older version in preference for the newer one and update all the places it gets used (e.g. in the cartopy course)

_LATITUDE_FORMATTER = mticker.FuncFormatter(lambda v, pos:
_north_south_formatted(v))

def deprecated_formatter(old_formatter, old_formatter_name, new_formatter_name):

Choose a reason for hiding this comment

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

E302 expected 2 blank lines, found 1
E501 line too long (80 > 79 characters)

DeprecationWarning)
return old_formatter

LONGITUDE_FORMATTER = deprecated_formatter(

Choose a reason for hiding this comment

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

E305 expected 2 blank lines after class or function definition, found 1

_north_south_formatted(v))


def deprecated_formatter(old_formatter, old_formatter_name, new_formatter_name):

Choose a reason for hiding this comment

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

E501 line too long (80 > 79 characters)

@lbdreyer lbdreyer force-pushed the deprecate_old_ticker branch from b7a687e to 8c93811 Compare May 1, 2018 16:30
@dopplershift
Copy link
Contributor

Would it be better to use matplotlib's deprecation infrastructure rather than adding our own?

Copy link
Member

@ajdawson ajdawson left a comment

Choose a reason for hiding this comment

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

You should be wary because the two formatters are not entirely equivalent, and there are some potential pitfalls with the newer one (see #401 (comment)). The differences are subtle, but there may be use cases that the originals serve better.

There are also issues with your proposed deprecation implementation that need consideration.

return old_formatter


LONGITUDE_FORMATTER = deprecated_formatter(
Copy link
Member

Choose a reason for hiding this comment

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

You are calling the function immediately when the module is imported, so this warning will always be shown regardless of whether the user actually wants to use LONGITUDE_FORMATTER or LATITUDE_FORMATTER. I think this will create too much noise and not target those people you want to see the warning.

@pp-mo
Copy link
Member

pp-mo commented May 2, 2018

@dopplershift Would it be better to use matplotlib's deprecation infrastructure rather than adding our own?

Do you just mean using mplDeprecation, like matplotlib/matplotlib#7723 ?
Or is there more to it ?
I'm no expert, but I can't find anything about managing deprecation in their developer guide pages.

@dopplershift
Copy link
Contributor

Matplotlib has a matplotlib.cbook.deprecated decorator for marking classes/functions as deprecated.

Copy link
Member

@pelson pelson left a comment

Choose a reason for hiding this comment

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

Thanks @lbdreyer! 👍 for deprecating this.

There are two issues that need to be addressed:

  • DeprecationWarnings are hidden by default, thus unless you engage with them, you'll never know about this until they are actually removed from the codebase (in which case, we may as well just remove them now and be done with it). A UserWarning is fine with me.
  • Don't invoke the deprecation warning on module import

@pelson
Copy link
Member

pelson commented Dec 5, 2018

@lbdreyer - any chance you've got some time to spin this back up? Would be good to target this for v0.18.

@lukelbd
Copy link
Contributor

lukelbd commented May 11, 2020

Wouldn't this PR be suitable for the next release?

I think the pitfall mentioned in #401 (comment) is no longer relevant in version 0.18:

if xformatter is None:
if isinstance(crs, cartopy.crs.PlateCarree):
xformatter = LongitudeFormatter(dms=dms)
else:
xformatter = classic_formatter()
#: The :class:`~matplotlib.ticker.Formatter` to use for the lon labels.
self.xformatter = xformatter
if yformatter is None:
if isinstance(crs, cartopy.crs.PlateCarree):
yformatter = LatitudeFormatter(dms=dms)
else:
yformatter = classic_formatter()
#: The :class:`~matplotlib.ticker.Formatter` to use for the lat labels.
self.yformatter = yformatter

Matplotlib's ScalarFormatter has always had the same limitation (you cannot re-use ScalarFormatter on multiple axes), but since ScalarFormatter is meant to be enabled automatically / untouched by 99.9% of users very few people run into this problem (and presumably being axis-specific allows you to design a more intelligent auto-formatting algorithm).

Now that version 0.18 makes LatitudeFormatter and LongitudeFormatter the default formatters for gridline labels, very few users will have to work with them directly / run into this problem.

One thing you could also do is add xformatter_kw and yformatter_kw keyword argument dictionary-arguments to cartopy.mpl.GridLiner(). These arguments would get passed to LatitudeFormatter and LongitudeFormatter on instantiation, which lets users customize the tick format and further avoids requiring people to work with the classes directly. Matplotlib uses this _kw convention in several places, e.g. pyplot.subplots().

@CLAassistant
Copy link

CLAassistant commented Apr 8, 2024

CLA assistant check
All committers have signed the CLA.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants