Skip to content

Allow timedelta to be converted to an ordinalf #9120

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 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions lib/matplotlib/dates.py
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,17 @@ def _dt64_to_ordinalf(d):
return dt


def _tdelta_to_ordinalf(tdelta):
Copy link
Member

Choose a reason for hiding this comment

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

Two objections:

  1. Despite the superficial and subject matter similarities, I'm not convinced that timedelta is necessarily best handled in dates.py. Date-time instances are complicated because their natural domain is a line in a non-decimal, often non-monotonic number system with complicated unit system. timedelta specifically, because it only handles durations whose length is invariant with translation along the number line, is effectively a convenience wrapper around an integer number of seconds, with almost none of the complexity of datetimes. If there's a general units framework, I think timedelta likely fits in there much better than lumping it in with dates.py. I'm open to being convinced that timedelta works best in dates.py

  2. It's not clear to me why the base unit for this is ordinal days. They don't really need to be compatible with how datetimes are handled, and the more natural base unit would be some unit like seconds, microseconds, nanoseconds, etc. np.timedelta64 has some complicated unit behavior, but the base type seems to be microseconds.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this has to be this way in order to be compatible with how we handle datetimes; the base unit has to the be the same if e.g. I want to plot something that has a timedelta width on a datetime axis.

Copy link
Member

Choose a reason for hiding this comment

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

@dstansby I think the question of having a timedelta width on a datetime axis was solved by #9072. That's very different from plotting a timedelta on a datetime axis.

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem has come up again in #11290 - I see no reason to use ordinal days - we have to use something, and using ordinal days would make our lives much easier with respect to plotting widths/heights on datetime axes.

Copy link
Member

Choose a reason for hiding this comment

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

I haven't had a chance to really look at and grok how the units framework works. I suppose it's possible that units are inferred from type and that everything gets translated into a number under the hood and plotted on the same plot, in which context I guess defining any mapping from timedelta to that single number line is fine because these things shouldn't be plotted at the same time but matplotlib isn't enforcing that in any way.

That said, from some preliminary tests, plotting arbitrary non-datetime points on a datetime axis currently fails with a message like:

ValueError: view limit minimum -4.95 is less than 1 and is an invalid Matplotlib date value. This often happens if you pass a non-datetime value to an axis that has datetime units

Sounds like it's a mix of the two, where you could plot non-datetime values if you got them right (because it's not enforced), but it's unlikely enough to work that there's a dedicated error message for it.

I think the right course forward for #11290 is to finish the work started in #9072 of defining all spans as start + span, such that types with relative but no absolute semantics or meaning (like datetime.timedelta and dateutil.relativedelta) don't cause errors.

"""
Convert :mod:`timedelta` to total days. Return value is a :func:`float`
"""
return tdelta.total_seconds() / SEC_PER_DAY


# a version of _tdelta_to_ordinalf that can operate on numpy arrays
_tdelta_to_ordinalf_np_vectorized = np.vectorize(_tdelta_to_ordinalf)


def _from_ordinalf(x, tz=None):
"""
Convert Gregorian float of the date, preserving hours, minutes,
Expand Down
7 changes: 7 additions & 0 deletions lib/matplotlib/tests/test_dates.py
Original file line number Diff line number Diff line change
Expand Up @@ -641,3 +641,10 @@ def test_tz_utc():
def test_num2timedelta(x, tdelta):
dt = mdates.num2timedelta(x)
assert dt == tdelta


def test_timedelta_ordinalf():
Copy link
Member

Choose a reason for hiding this comment

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

I have a religious objection to this test. I don't believe that the private interface should be explicitly tested like this (this is in fact not possible in less permissive languages). You can see the various answers on this SO thread for a list of reasons why not.

I would suggest implementing some part of the public interface that uses this _tdelta_to_ordinalf(dt) function and writing tests for that.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll try and put a proper plotting test with timedeltas in.

# Check that timedeltas can be converted to ordinalfs
dt = datetime.timedelta(seconds=60)
ordinalf = mdates._tdelta_to_ordinalf(dt)
assert ordinalf == 1 / (24 * 60)