Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Closed

Conversation

dstansby
Copy link
Member

Replaces#8730 and is a part of fixing#8869

@dstansbydstansby added this to the2.2 (next next feature release) milestoneAug 29, 2017
@dstansbydstansby mentioned this pull requestAug 29, 2017
6 tasks
@@ -474,3 +474,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 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_tdelta_to_ordinalf(dt) function and writing tests forthat.

Copy link
MemberAuthor

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.

@@ -239,6 +239,17 @@ def _to_ordinalf(dt):
_to_ordinalf_np_vectorized = np.vectorize(_to_ordinalf)


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 thattimedelta is necessarily best handled indates.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 itonly 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 ofdatetimes. If there's a general units framework, I thinktimedelta likely fits in there much better than lumping it in withdates.py. I'm open to being convinced thattimedelta works best indates.py

  2. It's not clear to me why the base unit for this is ordinaldays. 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
MemberAuthor

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 atimedelta width on adatetime 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 atimedelta width on adatetime axis was solved by#9072. That's very different from plotting atimedelta on adatetime axis.

Copy link
MemberAuthor

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 definingany mapping fromtimedelta to that single number line is fine because these things shouldn't be plotted at the same time butmatplotlib 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 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 asstart + span, such that types with relative but no absolute semantics or meaning (likedatetime.timedelta anddateutil.relativedelta) don't cause errors.

@jklymak
Copy link
Member

See#12863 for another implimentation of this....

@jklymakjklymak modified the milestones:v3.1.0,v3.2.0Feb 7, 2019
@jklymak
Copy link
Member

#12863 didn't pass muster either, so this is still something that could be done if someone has the interest. OTOH, I'd thionk timedeltas should just be converted to some sort of float before plotting...

@timhoffmtimhoffm modified the milestones:v3.2.0,v3.3.0Aug 16, 2019
@QuLogicQuLogic modified the milestones:v3.3.0,v3.4.0May 27, 2020
@jklymak
Copy link
Member

@dstansby do you think this is still something folks would want?

@dstansby
Copy link
MemberAuthor

Yes, I do. It needs careful tests and checks, but would definitely be useful. Happy to try and dig it out in the next week or so.

@jklymakjklymak marked this pull request as draftJuly 23, 2020 20:25
@dstansby
Copy link
MemberAuthor

I actually don't really have time to pick this up at the moment. Will close the PR but leave the branch if anyone else wants to pick it up in their own PR.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@pgansslepgansslepganssle left review comments

Assignees
No one assigned
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

6 participants
@dstansby@jklymak@pganssle@tacaswell@QuLogic@timhoffm

[8]ページ先頭

©2009-2025 Movatter.jp