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

WIP: Lockout new converters Part 2 #9776

Closed
wants to merge 1 commit into from

Conversation

@jklymak
Copy link
Contributor

jklymak commented Nov 13, 2017

PR Summary

See #9713 and #9736

This version, which supplants #9736 makes an axis only have a DefaultConverter if the first thing plotted on it is a float array. This allows the units converting machinery to pass through None for things like axis limits, but stop Dates or Categoricals from being plotted on the same axis as floats.

Still a WIP, but posting to help w/ #9774

Now changed so that the first units used are set for the axis, and further convert calls are sent to the same converter. If the converter wants to handle the data type it can. This makes mixed-use calls, i.e.

ax.plot(['a', 'b', 'c'])
ax.plot([1, 'd', 3])

work so long as the converter can handle the data in the second call.

Helps issues like #10147

PR Checklist

  • More-informative error handling
  • Has Pytest style unit tests
  • Code is PEP 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
@jklymak jklymak mentioned this pull request Nov 13, 2017
0 of 6 tasks complete
@jklymak

This comment has been minimized.

Copy link
Contributor Author

jklymak commented Nov 13, 2017

@anntzer Updated re discussion in #9774. This now allows set_xlim etc, before data is plotted:

This works:

import numpy as np
import matplotlib.pyplot as plt
import matplotlib.dates as mdates
import datetime

base = datetime.datetime.today()
time = [base - datetime.timedelta(days=x) for x in range(0, 3)]
data = np.random.rand( 3)

fig, ax = plt.subplots(tight_layout=True)
ax.set_ylim(time[0], time[1])
ax.plot( data, time)

This errors:

fig, ax = plt.subplots(tight_layout=True)
ax.set_ylim(0., 1.)
ax.plot( data, time)
@jklymak

This comment has been minimized.

Copy link
Contributor Author

jklymak commented Nov 21, 2017

This version doesn't lockout data that would be registered by new data, it just uses the original converter first registered for the axis. Its up to the converter to handle the new data.

For instance, if we call:

base = datetime.datetime(2017, 1, 1, 2, 3, 10)
time = [base + datetime.timedelta(days=x) for x in range(0, 3)]
ax.plot([1., 2.], [1., 3.])
ax.plot(time) 

the call will fail. But:

base = datetime.datetime(2017, 1, 1, 2, 3, 10)
time = [base + datetime.timedelta(days=x) for x in range(0, 3)]
ax.plot(time) 
ax.plot([1., 2.], np.array([1., 3.]) + 736330)

will succeed.

@story645

This comment has been minimized.

Copy link
Member

story645 commented Nov 21, 2017

How does this work in a sharex/sharey situation? Would plots in ax2 pick up the convertors (and mappings) from ax1?

@jklymak

This comment has been minimized.

Copy link
Contributor Author

jklymak commented Nov 21, 2017

Good question, I'm not sure. Do they do that now? Its doable, but I didn't do anything to make it happen.

@jklymak jklymak mentioned this pull request Dec 28, 2017
0 of 6 tasks complete
@@ -1430,7 +1434,15 @@ def update_units(self, data):
if *data* is registered for unit conversion.
"""

if self.converter is not None:
_log.debug('Axis already has a converter')
return True

This comment has been minimized.

Copy link
@dstansby

dstansby Dec 28, 2017

Member

I think this method should only return True if a converter is registered and data can be converted using that converter.

@@ -1011,7 +1014,7 @@ def cla(self):
else:
self.yaxis._set_scale('linear')
try:
self.set_ylim(0, 1)
self.set_ylim(0, 1, _converter=False)

This comment has been minimized.

Copy link
@dstansby

dstansby Dec 28, 2017

Member

Can kwargs be private? (I've never seen this before so I have no idea)

This comment has been minimized.

Copy link
@pganssle

pganssle Dec 28, 2017

Member

From a philosophical standpoint, nothing can be private in the panopticon that is Python symbol visibility, but here is an example from pytz of a private kwarg (though I think popping it from **kwargs so it doesn't show up under inspection is a better course to take).

This comment has been minimized.

Copy link
@anntzer

anntzer Dec 28, 2017

Contributor

I think it is pretty clear that a kwarg with an underscore is intended as private.

This comment has been minimized.

Copy link
@dstansby

dstansby Dec 28, 2017

Member

Maybe the better question is, will an underscored kwarg appear in the documentation? (I'm not convinced in this case that the kwarg is needed anyway)

This comment has been minimized.

Copy link
@pganssle

pganssle Dec 28, 2017

Member

@dstansby Looks to me like the way it's implemented, it's just being pulled from the kwargs dictionary, it's not an actual declared keyword argument, so you'd need a pretty smart document generator to even figure out that passing _converter to this would be meaningful.

This comment has been minimized.

Copy link
@jklymak

jklymak Jan 3, 2018

Author Contributor

Just to justify this kwarg - the point of this PR is that the first time you conver units on an axis, the converter gets set. This happens on set_xticks, set_xlims or any of the plotting calls.

cla calls set_xlim but its not doing so because it has any data that might have units. So this kwarg is just an easy way to say "set the limit, but don't assign the units because this is just a placeholder".

We could, alternately, make a new method (_set_xlim_placeholder) that did the same thing, if that is better than a hidden kwarg?

This comment has been minimized.

Copy link
@anntzer

anntzer Jan 3, 2018

Contributor

I think either way is fine.

self._process_unit_info(xdata=(left, right))
left = self._validate_converted_limits(left, self.convert_xunits)
right = self._validate_converted_limits(right, self.convert_xunits)
if _converter:

This comment has been minimized.

Copy link
@dstansby

dstansby Dec 28, 2017

Member

What's the purpose of this if statement?

This comment has been minimized.

Copy link
@jklymak

jklymak Jan 3, 2018

Author Contributor

As above. Its because cla needs the set_x/ylim call. I tried getting rid of it, but bad things seemed to happen.

@jklymak jklymak force-pushed the jklymak:lockoutnewconvertersNew branch from 36b8483 to 81bf74e Jan 3, 2018
@jklymak jklymak added the categorical label Jan 3, 2018
@efiring

This comment has been minimized.

Copy link
Member

efiring commented Jan 8, 2018

As we discussed after today's meeting: I think that any lockout machinery should apply only to converters, not to the native floats to which unitized quantities are converted for plotting. For example, in making a plot of categoricals, one should remain able to place lines or any other artists on the Axes using native floats. Similarly, one should remain able to plot Datetimes and mpl datenums on the same Axes. This should be possible regardless of the order in which artists are added.

@tacaswell

This comment has been minimized.

Copy link
Member

tacaswell commented Jan 12, 2018

@jklymak

This comment has been minimized.

Copy link
Contributor Author

jklymak commented Jan 19, 2018

@efiring, just to rephrase: the converter would be locked to the axis. However, the converter could still choose to deal w/ floats in a self-consistent way. So, the datetime converter could still be set to allow floats through, however, it might perform its own checks on them.

What that would preclude however, is calling plot with a float first and then with a date later and expecting the date to work. To get around that, there should probably be a mechanism to manually set the converter to an axis.

@jklymak

This comment has been minimized.

Copy link
Contributor Author

jklymak commented Mar 18, 2018

I still think this is a good idea, but will close until some more thought goes into units support.

@jklymak jklymak closed this Mar 18, 2018
@jklymak jklymak deleted the jklymak:lockoutnewconvertersNew branch Mar 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.