FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

WIP: Lockout new converters Part 2 by jklymak · Pull Request #9776 · matplotlib/matplotlib · GitHub

Repository navigation

WIP: Lockout new converters Part 2 - #9776

Closed
jklymak wants to merge 1 commit into
matplotlib:masterfrom
jklymak:lockoutnewconvertersNew
Closed

jklymak wants to merge 1 commit into
matplotlib:masterfrom
jklymak:lockoutnewconvertersNew

Conversation

jklymak commented Nov 13, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

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 mentioned this pull request Nov 13, 2017
6 tasks

jklymak commented Nov 13, 2017

Copy link
Copy Markdown
Member Author

@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 commented Nov 21, 2017

Copy link
Copy Markdown
Member Author

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.

Copy link
Copy Markdown
Member

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

jklymak commented Nov 21, 2017

Copy link
Copy Markdown
Member Author

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

dstansby added this to the v2.2 milestone Nov 22, 2017
jklymak mentioned this pull request Dec 28, 2017
6 tasks
Comment thread lib/matplotlib/axis.py Outdated

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

self.yaxis._set_scale('linear')
try:
self.set_ylim(0, 1)
self.set_ylim(0, 1, _converter=False)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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)

pganssle Dec 28, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

What's the purpose of this if statement?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

efiring commented Jan 8, 2018

Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Member

attn @jrevans @TD22057

tacaswell modified the milestones: v2.2, v3.0 Jan 12, 2018

jklymak commented Jan 19, 2018

Copy link
Copy Markdown
Member Author

@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 commented Mar 18, 2018

Copy link
Copy Markdown
Member Author

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

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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.

7 participants


Back | FazBrowse Home | New Git URL