Repository navigation
Fix datetime accuracy - #9679
Conversation
|
Work on #9532 suggests that this does not completely fix the problem; I don't yet know why, but TimeDelta -> datetime.timedelta -> TimeDelta slips about ten microseconds. The other roundtrip appears to be fine, somehow? I'll update here when I've sorted it. |
No actually it's fine, I had accidentally converted through a Time default format's |
|
I guess if it isn't going into 4.0 it needs a changelog entry? |
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
Looks good! Small comments only.
Maybe post here an example where this fails; I'd quite happily merge this unless there is a real show-stopper, as it would seem to give good improvements already.
| for jd, out in iterator: | ||
| out[...] = datetime.timedelta(days=jd.item()) | ||
| for jd1, jd2, out in iterator: | ||
| jd1_, jd2_ = day_frac(jd1.item(), jd2.item()) |
There was a problem hiding this comment.
The .item() shouldn't be necessary here. Also, would think that the day_frac is not needed either, since timedelta would do it internally (but haven't tested).
There was a problem hiding this comment.
The existing code used .item() but I don't know why, so I left it.
Didn't we have this discussion, that astropy does not guarantee anything about the values of jd1 and jd2? Specifically, custom formats pass through no checking, and only some code will break when presented with unusual values here. Scale conversion could also produce non-compliant jd1 and jd2, though I think the fix for that was merged. My argument was that having a near-invariant was worse than not providing any invariant, because code would depend on it, as your suggestion does, and then some obscure bits of code would break, in this case in subtle ways.
There was a problem hiding this comment.
Yes, we had. My suggestion was simply to check whether timedelta does the rounding for one and thus takes care of the case where jd1 and jd2 are no longer quite as expected. And it does in fact do this, so I guess here one is mostly guarding against the case where jd1 and jd2 would be swapped or so.
In any case, it doesn't really matter - an object array of timedelta is going to be so slow to deal with that an extra call to day_frac will not add much.
There was a problem hiding this comment.
p.s. Let's remove the .item() - I'm fairly sure it is a copy&paste mistake from the other direction, where, since one deals with objects, it makes sense to get the actual timedelta out.
There was a problem hiding this comment.
I sort of want to create a custom format that constructs non-normalized jd1/jd2 and see how much stuff subtly breaks. The ERFA test case in test_sidereal is one example, this suggested implementation is another, I wonder how much else is baked in to our code base? Not everything, most stuff would be fine.
| assert abs(t2.jd2) < 0.5 or t2.jd1 % 2 == 0 | ||
|
|
||
|
|
||
| def test_datetime_difference_agrees_with_timedelta_no_hypothesis(): |
There was a problem hiding this comment.
Nitpick, but omit _no_hypothesis() (at the very least for this PR!).
There was a problem hiding this comment.
It's all a bit awkward because I don't trust this to be a useful test; it's the hypothesis version that is the real test, and I wouldn't have recommended merging this until it passed the hypothesis testing. So this clone is largely decorative.
There was a problem hiding this comment.
Don't quite disagree - this was a test that showed the previous implementation was wrong, so it is good to guard against regression regardless!
There was a problem hiding this comment.
Well, but I think it takes the hypothesis tests to be confident of avoiding regression. And they exist, maintained in parallel to this one.
|
I think this is a bug fix, and thus needs a changelog entry. Since it is not release critical, let's aim for 4.0.1 - @bsipocz mentioned earlier she'll try to put in anything from 4.0.1 that is in before the release critical fixes have been made, so with a bit of luck, it will still be there from day 1. |
|
@mhvk - timing wise I aim for the release to be finalized during the coordination meeting (well, definitely not sooner, looking at the wiki we still miss some important testing feedback). |
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
Looks all OK to me now. Thanks!
Fix TimeDelta accuracy issue with datetime.timedelta
4 participants
Fixes #9579
Description
This pull request is to address accuracy limitations in datetime <-> Time conversion revealed by PR #9532 and reported in bug #9579 .
Fixes #9579