Skip to content

[WIP] Add the ability for unit converters to convert back to data with units #12270

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

Closed
wants to merge 5 commits into from

Conversation

dstansby
Copy link
Member

@dstansby dstansby commented Sep 25, 2018

Inspired by #7462, and not-so-recent units discussions on the mailing list, I have made a minimal implementation of having an un_convert method for ConversionInterface classes. The benifit of this is that Matplotlib can re-convert from "axis coordinates" to "data/unit coordinates", and return data with units. This is particularly useful for things like ginput.

I will write this up more in the future, but for to keep a paper trail this is a pre-requisite for a accepts_units decorator (see #10411 for an attempt at this) that would automatically catch issues such as #15332 ,

@jklymak
Copy link
Member

jklymak commented Sep 25, 2018

Many of the converters convert from different units to matplotlib values. I.e. the date converters. So how will you invert if the inversion is non-unique?

As pointed out a few times, I think any co-ordinate that gets unitized should go ahead and be converted, but we should pack the original in w/ the conversion. Then we can always get back to where we started. I.e. x=dict(orig=Xorig, converted=currentconversion)

@dstansby
Copy link
Member Author

Can you give an example of the different date units?

The way I've set it up, doing how the back-conversion is done is entirely up to the implementation of the ConversionInterface. If needed, what is returned can depend on the units attribute of the axis, which can change from e.g. m to cm.

Although that is potentially a good idea, it doesn't cover cases where the conversion back is not being done on a data point, e.g. xlim/ylim are almost never the same as an actual data point passed in.

@dstansby dstansby force-pushed the un-convert branch 3 times, most recently from 3e22cfc to 78dda48 Compare September 25, 2018 17:06
@jklymak
Copy link
Member

jklymak commented Sep 25, 2018

The date converter will accept datetime.datetime, datetime.date and numpy.datetime64 right now. If we convert to ordinal (days since epoch) as we do now, how will we know the correct inversion? Practically this could have an effect if the user overrides the datetime64 conversion, but not the datetime.datetime, and converts, but then the inversion puts the date back in datetime units.

More to the point, if we change the units of the axis, inverting and then re-converting is not garaunteed to be correct if we don't get the original data.

I'm not sure I follow the xlim/ylim issue. I don't see any reason xlim/ylim can't be wrapped in the same way as other data.

@dstansby
Copy link
Member Author

That makes sense; we can just make different copies of each ConversionInterface, that inherit everything from a DateConverter, but just have different methods to convert the units back.

@dstansby dstansby force-pushed the un-convert branch 3 times, most recently from e50d728 to 79acf30 Compare September 26, 2018 09:28
@dstansby dstansby changed the title Add the ability for unit converters to convert back to data with units [WIP] Add the ability for unit converters to convert back to data with units Sep 26, 2018
@dstansby dstansby force-pushed the un-convert branch 2 times, most recently from ec80e48 to 15ddfa9 Compare October 2, 2018 09:42
@dstansby
Copy link
Member Author

dstansby commented Oct 4, 2018

Still needs:

  • API changes (or what's new?)
  • Separate un-converters for each of the date types

@jklymak jklymak modified the milestones: v3.1.0, v3.2.0 Feb 26, 2019
@dstansby dstansby force-pushed the un-convert branch 2 times, most recently from 61c05d2 to 67bde14 Compare May 24, 2019 14:55
@dstansby dstansby force-pushed the un-convert branch 2 times, most recently from 22f7fed to 5449bcd Compare July 22, 2019 08:04
@dstansby dstansby modified the milestones: v3.2.0, v3.3.0 Aug 17, 2019
@dstansby dstansby force-pushed the un-convert branch 2 times, most recently from 54c996e to 3e837dd Compare August 17, 2019 10:46
@dstansby dstansby force-pushed the un-convert branch 3 times, most recently from 985b499 to 35a73a2 Compare September 27, 2019 11:39
@jklymak
Copy link
Member

jklymak commented Sep 27, 2019

@dstansby, I think this should be discussed more before more work is done. As I understand it, the plan for 4.0 is to have a new way of carrying the data around inside Matplotlib. That may be a pipe dream, in which case something like this may be a good half measure, but we should make sure this fits in the roadmap? @dopplershift @tacaswell were particularly interested in better units support.

@dstansby
Copy link
Member Author

Thanks for pointing that out - is there a roadmap for 4.0 or anything written down anywhere about the changes to data-carrying? Either way I'm kind of happy to carry on playing with this and writing it up on a zero-merge-expectation basis.

@jklymak
Copy link
Member

jklymak commented Sep 29, 2019

That seems reasonable. There is a paper document. https://paper.dropbox.com/doc/Matplotlib-4.0-WTYwd0NQaSHTjtLUZwkNx

@QuLogic QuLogic modified the milestones: v3.3.0, v3.4.0 May 2, 2020
@jklymak jklymak marked this pull request as draft July 23, 2020 16:34
@dstansby
Copy link
Member Author

I think there's another version of this kicking around somewhere, and it's clear that something like this needs some more careful thought and design, so I'm going to close this.

@dstansby dstansby closed this Jul 28, 2020
@QuLogic QuLogic removed this from the v3.4.0 milestone Mar 16, 2021
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.

4 participants