update units.ConversionInterface to have `to_numeric` and `from_numeric` #16922

terencehonles · 2020-03-26T17:53:52Z

This change updates matplotlib.units.ConversionInterface to have a to_numeric instead of a convert method, and adds a from_numeric method. The method convert is renamed to to_numeric and similarly named methods are updated accordingly. The renaming is done to make convert less ambiguous with a forward and backward conversion on the ConversionInterface.

Adding a from_numeric method standardizes the way to convert back to a native object from an internal numeric value. This allows registered converters to provide more specific conversions which will allow formatters and locators to provide an implementation which is not as tied to the internal values Matplotlib uses.

PR Summary

PR Checklist

Has Pytest style unit tests
Code is Flake 8 compliant
New features are documented, with examples if plot related
Documentation is sphinx and numpydoc compliant
Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

story645 · 2020-03-26T18:16:42Z

So this is a major overhaul that might create deprecation issues with the third party packages the conversation interface is aimed at. Is there buy in/packages that need this functionality? And can it be achieved w/o renaming convert?

terencehonles · 2020-03-26T19:11:59Z

So this is a major overhaul that might create deprecation issues with the third party packages the conversation interface is aimed at. Is there buy in/packages that need this functionality? And can it be achieved w/o renaming convert?

Thanks for the quick response, in this PR as proposed I have marked convert as a pending deprecation (because I think this conversation is needed, and possibly with packages that implement the interface). I have made everywhere that previously called convert backwards compatible and it would be up to Matplotlib to determine the deprecation timeline. So other than raising warnings, this change should not affect implementers of the ConversionInterface, and it would be safe to provide both convert and to_numeric.

Regarding the buy in, I am not sure, but I believe this change is needed because pandas uses a different date converter. Pandas does subclass the AutoDate{Formatter,Locator} (see pandas) and it provides those in its AxisInfo, but when using the AutoDate{Formatter,Locator} provided by Matplotlib (which would seem like it should work) or the ones provided by pandas I'm getting an exception ValueError: year 4320917 is out of range. When debugging the data, the data is a valid UNIX timestamp, but not a valid ordinal, and is caused by the DateLocator calling num2date when the value needs to be converted into an object, but since the DateLocator has no way to determine what the value was before it converts it incorrectly.

I believe my issue is related to pandas-dev/pandas#18344, and now that I have prepared this PR, I will be trying to make sure the tests pass, and testing my code with a patch to pandas that implements a from_numeric.

Regarding the name change, I believe this could be made without a name change, but it sacrifices the readability and makes the functions slightly ambiguous. Arguably you could make the methods be convert and unconvert, but which direction are they converting? Without reading the documentation it feels likely that someone might pick the wrong method, and I believe it would be better to start transitioning away from an ambiguous name, but potentially always leaving this a pending deprecation and falling back to the old method.

timhoffm

Thanks @terencehonles. This is rather a big step in the API, which will have to be thought through carefully. Please be prepared, that the review process may take some time. While I haven't looked into the details yet, the basic idea looks reasonable.

Apart from the individual comments, please address the following:

Change all deprecations to 3.3, because 3.2 is already out.
Make all deprecations pending. While you wrote you did in the PR comment, I don't see it in the code.
This needs an API change note at doc/api/next_api_changes.rst for the deprecations. The new to and from methods should also be announced, probably best at doc/users/next_whats_new.

lib/matplotlib/axis.py

timhoffm · 2020-03-26T23:40:27Z

lib/matplotlib/units.py

@@ -51,7 +51,7 @@ def default_units(x, axis):
 from matplotlib import cbook


-class ConversionError(TypeError):
+class ConversionError(ValueError):


This would be an API change. Not sure it's needed or even desirable. It's raised when convert_units(x) cannot process the argument. Typically that should happen rather if x has an unsupported type than value.

Looking at where it is used

matplotlib/lib/matplotlib/axis.py

Lines 1466 to 1470 in c30129b

try:

ret = self.converter.convert(x, self.units, self)

except Exception as e:

raise munits.ConversionError('Failed to convert value(s) to axis '

f'units: {x!r}') from e

I personally would have thought that should have been a ValueError, but I can understand why in some cases you can argue one way or another for ValueError vs TypeError. I do agree this would be a breaking change if someone is expecting to catch a TypeError instead of a ConversionError and maybe you feel strongly enough that this shouldn't change. This is partially changed because I'm more under the impression it should be a ValueError, and I did end up using the exception a couple more places and throwing this instead of a ValueError. The appropriate fix may be creating two different errors or maybe just documenting an API change.

So in summary: I did this because it didn't seem like it would be a big deal and I am using the exception more widely than it was used before (new use cases with the new method I added, and for existing errors that were both ValueErrors and seemed like they should have actually fallen under a ConversionError)

lib/mpl_toolkits/mplot3d/axes3d.py

timhoffm · 2020-03-26T23:46:13Z

tests.py

@@ -1,29 +0,0 @@
-#!/usr/bin/env python


Please do not add unrelated changes to large PRs like this one.

Apart from whether this should be removed or not, any unrelated change makes reviewing harder, which may delay the review process, or even discourage reviewers from taking on the PR.

Instead, please keep the PR as focused as possible and submit everything else as a separate PR.

These changes were simply to allow me to more easily test my changes (other than using docker, I only have Python 2.7 and 3.8 on my system). I created PR #16986 which cherry picks these changes.

Sorry, I guess this comment more belongs on tox.ini the change on this file was that I wasted a lot of time trying to figure why this test failed, and I didn't realize there was a diff file that was generated. I updated this test util to make it more obvious, but I can also pull this out to a different PR

Actually this comment is on a deleted file, I will pull this and the change I was talking about ^^^ into a different PR.

It also looks like I was wrong about deleting this file. It looks like it expects you have already installed matplotlib in pip editable mode, and I'll suggest that change instead of deleting this file (I looked at its commit log and it has been updated recently so I looked a bit closer)

tox.ini

This change updates `matplotlib.units.ConversionInterface` to have a `to_numeric` instead of a `convert` method, and adds a `from_numeric` method. The method `convert` is renamed to `to_numeric` and similarly named methods are updated accordingly. The renaming is done to make `convert` less ambiguous with a forward and backward conversion on the `ConversionInterface`. Adding a `from_numeric` method standardizes the way to convert back to a native object from an internal numeric value. This allows registered converters to provide more specific conversions which will allow formatters and locators to provide an implementation which is not as tied to the internal values Matplotlib uses.

jklymak · 2020-03-31T21:09:28Z

There have been lots of discussion about fixing units in Matplotlib, including the problem of calculating the inverse. Currently my understanding is that there is a new data model under development that will address this by @tacaswell and @story645. How does this PR fit into those plans? I’m also not in favour of renaming a long-standing method for purely aesthetic reasons.

terencehonles · 2020-03-31T22:18:02Z

There have been lots of discussion about fixing units in Matplotlib, including the problem of calculating the inverse. Currently my understanding is that there is a new data model under development that will address this by @tacaswell and @story645. How does this PR fit into those plans? I’m also not in favour of renaming a long-standing method for purely aesthetic reasons.

I'm not aware of those plans, I can possibly help with those plans if needed. I was mainly taking a stab at this because it looks like it has been a longstanding issue and it affects me directly. So rather than just patching it internally or updating pandas's formatters/locators I figured something more generic and less surprising would be ideal. If there is an email thread/PR/issue already I can be added to it.

terencehonles · 2020-03-31T22:20:50Z

Thanks @terencehonles. This is rather a big step in the API, which will have to be thought through carefully. Please be prepared, that the review process may take some time. While I haven't looked into the details yet, the basic idea looks reasonable.

Apart from the individual comments, please address the following:

Change all deprecations to 3.3, because 3.2 is already out.

Make all deprecations pending. While you wrote you did in the PR comment, I don't see it in the code.

This needs an API change note at doc/api/next_api_changes.rst for the deprecations. The new to and from methods should also be announced, probably best at doc/users/next_whats_new.

I updated the deprecations, the pending flag was only on the conversion interface, but I added it to all of the deprecations I added in this PR. Regarding the documentation I figured starting a discussion about the change would be appropriate before spending any more time than I have.

I appreciate you looking at it and @jklymak and any others that might have the chance to offer some input.

jklymak · 2020-03-31T22:51:03Z

I'm not aware of those plans, I can possibly help with those plans if needed

I'm sure that would be appreciated.

https://github.com/matplotlib/matplotlib/labels/units

Note that there are other solutions to pandas-dev/pandas#18344 under https://github.com/matplotlib/matplotlib/labels/Date%20handling, in particular #15008

tacaswell · 2020-04-06T18:56:20Z

The data model work is going to intersect with the units work by better managing the life cycle of when it gets called, but should be relatively agnostic to how we spell the unit conversion.

tacaswell · 2020-04-06T19:38:18Z

We talked about this on the dev-call this week (.https://hackmd.io/-fJ2hZcJQz2gfbqzabrZMQ?view#Units-change). We have several concerns:

the renames are making in very hard to think about and review this set of changes. While we appreciate you work on this, can you back the deprecation and rename of convert out of this PR if you want to go forward with this. I think the same goes for the staticmethod -> classmethod changes.
in general, it is not clear that the "unconvert" is always well defined. For example if the user gives us both np.datetime64 or datetime.datetime we lose the information about which one we started as so don't know how to go back to what the user provided us. If this is going to go in the unconvert method is going to have to be optional for over and provide a way for the the converter to declare that it can't be inverted.

tacaswell

Please:

revert the rename API changes, we can discuss those in a separate PR
address concerns about converters that are not actually invertable.

github-actions · 2023-07-17T02:12:07Z

Since this Pull Request has not been updated in 60 days, it has been marked "inactive." This does not mean that it will be closed, though it may be moved to a "Draft" state. This helps maintainers prioritize their reviewing efforts. You can pick the PR back up anytime - please ping us if you need a review or guidance to move the PR forward! If you do not plan on continuing the work, please let us know so that we can either find someone to take the PR over, or close it.

terencehonles force-pushed the add-from-numeric-conversion-to-ConversionInterface branch 2 times, most recently from 5ccae25 to 80087a2 Compare March 26, 2020 18:00

anntzer added the topic: units and array ducktypes label Mar 26, 2020

terencehonles force-pushed the add-from-numeric-conversion-to-ConversionInterface branch from 80087a2 to 17c0e26 Compare March 26, 2020 18:35

terencehonles force-pushed the add-from-numeric-conversion-to-ConversionInterface branch from 17c0e26 to 408ecb6 Compare March 26, 2020 19:14

timhoffm reviewed Mar 26, 2020

View reviewed changes

terencehonles mentioned this pull request Mar 31, 2020

update tox.ini to match pythons supported and allow flags for pytest #16986

Merged

6 tasks

terencehonles force-pushed the add-from-numeric-conversion-to-ConversionInterface branch from 408ecb6 to e170f99 Compare March 31, 2020 20:42

terencehonles force-pushed the add-from-numeric-conversion-to-ConversionInterface branch from e170f99 to 2fe4da5 Compare March 31, 2020 21:02

terencehonles mentioned this pull request Mar 31, 2020

update testing helpers #16990

Merged

6 tasks

tacaswell added this to the v3.3.0 milestone Apr 6, 2020

tacaswell requested changes Apr 6, 2020

View reviewed changes

QuLogic modified the milestones: v3.3.0, v3.4.0 May 5, 2020

jklymak added status: needs rebase status: needs revision labels Jul 24, 2020

jklymak marked this pull request as draft July 24, 2020 03:07

anntzer mentioned this pull request Dec 7, 2020

Qt backend's Figure options to support axis units #19075

Closed

QuLogic modified the milestones: v3.4.0, v3.5.0 Jan 21, 2021

spine-o-bot mentioned this pull request Feb 4, 2021

Human-readable x-axis range for timeseries plots spine-tools/Spine-Toolbox#1014

Closed

tacaswell modified the milestones: v3.5.0, v3.6.0 Aug 5, 2021

timhoffm modified the milestones: v3.6.0, unassigned Apr 30, 2022

story645 modified the milestones: unassigned, needs sorting Oct 6, 2022

github-actions bot added the status: inactive Marked by the “Stale” Github Action label Jul 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update units.ConversionInterface to have `to_numeric` and `from_numeric` #16922

update units.ConversionInterface to have `to_numeric` and `from_numeric` #16922

terencehonles commented Mar 26, 2020

story645 commented Mar 26, 2020

terencehonles commented Mar 26, 2020

timhoffm left a comment •

edited

Loading

timhoffm Mar 26, 2020

terencehonles Mar 31, 2020

timhoffm Mar 26, 2020

terencehonles Mar 31, 2020

terencehonles Mar 31, 2020

terencehonles Mar 31, 2020

terencehonles Mar 31, 2020

jklymak commented Mar 31, 2020

terencehonles commented Mar 31, 2020

terencehonles commented Mar 31, 2020

jklymak commented Mar 31, 2020 •

edited

Loading

tacaswell commented Apr 6, 2020

tacaswell commented Apr 6, 2020

tacaswell left a comment

github-actions bot commented Jul 17, 2023

	try:
	ret = self.converter.convert(x, self.units, self)
	except Exception as e:
	raise munits.ConversionError('Failed to convert value(s) to axis '
	f'units: {x!r}') from e

update units.ConversionInterface to have to_numeric and from_numeric #16922

Are you sure you want to change the base?

update units.ConversionInterface to have to_numeric and from_numeric #16922

Conversation

terencehonles commented Mar 26, 2020

PR Summary

PR Checklist

story645 commented Mar 26, 2020

terencehonles commented Mar 26, 2020

timhoffm left a comment • edited Loading

Choose a reason for hiding this comment

timhoffm Mar 26, 2020

Choose a reason for hiding this comment

terencehonles Mar 31, 2020

Choose a reason for hiding this comment

timhoffm Mar 26, 2020

Choose a reason for hiding this comment

terencehonles Mar 31, 2020

Choose a reason for hiding this comment

terencehonles Mar 31, 2020

Choose a reason for hiding this comment

terencehonles Mar 31, 2020

Choose a reason for hiding this comment

terencehonles Mar 31, 2020

Choose a reason for hiding this comment

jklymak commented Mar 31, 2020

terencehonles commented Mar 31, 2020

terencehonles commented Mar 31, 2020

jklymak commented Mar 31, 2020 • edited Loading

tacaswell commented Apr 6, 2020

tacaswell commented Apr 6, 2020

tacaswell left a comment

Choose a reason for hiding this comment

github-actions bot commented Jul 17, 2023

update units.ConversionInterface to have `to_numeric` and `from_numeric` #16922

update units.ConversionInterface to have `to_numeric` and `from_numeric` #16922

timhoffm left a comment •

edited

Loading

jklymak commented Mar 31, 2020 •

edited

Loading