Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Changes fromall commits
File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading.Please reload this page.
Jump to
Uh oh!
There was an error while loading.Please reload this page.
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -271,6 +271,17 @@ def _dt64_to_ordinalf(d): | ||
return dt | ||
def _tdelta_to_ordinalf(tdelta): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Two objections:
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 definingany mapping from That said, from some preliminary tests, plotting arbitrary non-datetime points on a datetime axis currently fails with a message like:
Sounds like it's a mix of the two, where youcould 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 | ||
""" | ||
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, | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -641,3 +641,10 @@ def test_tz_utc(): | ||
def test_num2timedelta(x, tdelta): | ||
dt = mdates.num2timedelta(x) | ||
assert dt == tdelta | ||
def test_timedelta_ordinalf(): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 factnot possible in less permissive languages). You can see the variousanswers on this SO thread for a list of reasons why not. I would suggest implementing some part of the public interface that uses this There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) |