-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Dates: Documentation, tests, and changed arithmetics for DateTime
#50816
Conversation
If the incorrect method can't be removed, at least it can be |
Yes, I suppose we can at least deprecate the What is the protocol for breaking changes anyway? Is any breaking change prohibited? Or do we run nanosoldier to see if any packages rely on the current behavior, and if not, then we can merge the change? |
There is no formal protocol, though if something is explicitly documented in the manual (a docstring is not sufficient), we try very hard not to break the behavior. If possible, deprecation for (upcoming) breaking changes should be done rather than doing the breaking change. |
Alright, thank you. How to proceed here then? I did not remove any methods, only added a potential |
Changing a non-error to an error is breaking, so my feeling is that the error path should not happen for now. Since |
787e31e
to
f5d2b8e
Compare
Can you see the new version, please? Instead of throwing an error, the methods now issue a deprecation warning. I put instructions into comments on what to do when the deprecation may turn into an error. |
I'd just deprecate the entire Micro-/Nanosecond method. Only every 1.000/1.000.000 micro/nanoseconds are perfectly convertible to milliseconds, and a one in a thousand or one in a million chance to NOT get an error from addition (when it seems preferable to convert explicitly in all cases either way) seems better. But then again, we don't have something like |
2184bef
to
b9835ba
Compare
Yes, that makes sense. It is done. |
b9835ba
to
f46a452
Compare
LGTM! |
Thank you! I see the tests passing locally, but the CI fails: https://buildkite.com/julialang/julia-master/builds/26597#0189d9f2-4590-4579-8d55-9a9208e78d17/775-1116 Do you have any idea what might be the reason? |
Likely |
But |
f46a452
to
96f00ae
Compare
Good point - Maybe we'll be able to see the actual error if you modify the test to
as currently there's an Looking at the rest of the file, it's a bit curious that the other deprecations in there are all commented out 🤔 |
Seeing the discussion in #29509, seems like the deprecations were just left like that for later 😬 |
Oh, and also see #37814. I could uncomment the depwarns (#37814 (comment)) in this PR, too? I should also annotate the deprecated methods with |
96f00ae
to
c471935
Compare
Yes, since Jeff mentioned to just uncomment them, go for it. Do make sure to replace the |
c471935
to
34931ac
Compare
I think I know what's happening. The buildkite tests run with # start with julia --depwarn=error
julia> using Test
julia> x = @test_deprecated Base.depwarn("", nothing);
julia> x
Test Passed
Thrown: ErrorException I am not sure how to use |
34931ac
to
36bffa2
Compare
The tests should pass now. We are testing that the deprecated methods are deprecated, but not their results. I think that's the best we can do right now since Because of the same reason, uncommenting the other |
Ah, that's a shame 😔 But well, nothing we can do about in this PR. Thanks for looking into it! The windows failure seems to come from SparseArrays, so should be unrelated to this PR. |
It would be great if you could cross post your stance to discourse, both so I don't accidentally misrepresent it and so that others can follow your argument without having to read through this very long PR. |
I think the Dates docs introductory section needs to be rewritten if this gets merged. For example
combined with
to my eyes implies that Date division returns a floating-point value, and that's not what this PR does. That whole section, with the analogy to Java, all the references to |
If the new interpretation is inconsistent with existing, documented behavior, to the degree that it would require a large rewrite of that documentation, isn't that a good indicator for this not being a good/correct interpretation at all? Are we allowed to change the interpretation of any datatype in a non-breaking release, as long as we also "fix" the documentation to remove any mention of the old interpretation? How much is the stability of our docs worth here? |
@timholy From my point of view, the reason not to do this is that a my understanding of a I am not cognoscent of all the issues around maintaining a stdlib, but it does strike me that changing truncation to rounding will change people's programs, so I feel merging this carries some risk of breaking code. I agree that getting feedback from heavy users of Dates is a good course of action. I don't know that I qualify as a 'heavy user', however. |
Thanks, @barucden! Note that all this does is switch from truncation to rounding. So |
Come on, there was a HUGE amount of discussion on discourse, you yourself said you want to remove yourself from this and now you just merge this? I thought we arrived at a consensus that increasing precision of Potentially rounding up is even worse than truncation, because now you can't guarantee that addition won't overcompensate! That is a big problem.. |
Triage discussed it and made a decision. This implemented the decision. A consensus by people who have been contributing to Julia for years, and who have an established track record in designing large, coherent software stacks trumps any discourse discussion. It is too bad that there is no way to make everyone happy here, but the amount of discussion over a tiny change is a bit excessive. Compare #31922, which also changed answers that Julia delivers for some operations but which somehow avoided becoming a big deal. |
And that decision by triage is completely intransparent, it's unknown who participated, what actually was discussed and how the conclusion was reached, whether the arguments brought forth in this PR have been addressed, whether there was anyone with your requested |
I also want to point out that you added the |
As far as I can tell, the impetus for the discourse discussion came from you: #50816 (comment). My only mention of the discourse discussion was in #50816 (comment). I guess if "few" = "3 or more" I should have waited until tomorrow, apologies. But really, the amount of discussion here is excessive. Switching from truncation to rounding switches the asymptotic error from scaling like |
There shouldn't be any error at all though, which is why the discourse discussion settled on "let's improve the precision, to capture |
Sure, that's why I gave you a thumbs up on reopening that issue. You're welcome to work on a PR that fixes it for good. This is a manifest improvement from where we are now, and merging it does nothing to prevent future improvements. |
No, it is not an improvement. Truncation is preferable to rounding - there certainly is an expectation that adding something to a |
Because if you add a million |
Timestamps are exact points in time (up to a given precision). They are not "approximate". "Approximately the right answer" is not ok for timestamps, especially if that error can accumulate to more than the stated precision of the timestamp. |
I'm afraid there's very little we seem to agree on. I wish it were different. I do hope you can channel your frustration into making further improvements to Dates that ensure users get something truly awesome before the 1.11 feature freeze forces this choice on them. |
I still believe that this option carries the the most mental burden to the user out of all presented. Whether or not it was the right choice to merge regardless, I think this
is clearly not true. Multiple users have weighed in on this thread and the Discourse one (even over half?) suggesting that they think rounding is not the right call (some disfavoring truncation as well, preferring an error)
and of course 9. and 10. you know how I and @Seelengrab feel In fact, as far as I can tell, there are essentially zero proponents of rounding except for whoever was on the triage call? |
Almost all of those are complaints that we lose data. I don't like that either. But without making a breaking change or promoting to a more-precise time type, there's literally nothing we can do about that---we can't actually disable those methods until Dates 2.0. But let's be very, very precise here. None of those comments address the following question: if you have to live in a world you hate, which is worse, truncation or rounding? This is a very narrow question, and the one that actually faces us in this PR. While all of us might rather not live in that world, this is what we've got to deal with. This PR does nothing except decrease the rate at which we lose data, and thus is orthogonal to all of the comments you quoted. #50785 is still open and I support that. Think of it this as a battle plan:
We just completed step 1. That doesn't mean we're not waiting for PRs to implement step 2. But depending on whether the solution is breaking or non-breaking, that could be multiple release cycles away. So rather than be held hostage to a never-ending discussion, let's get a simple win now and we can keep talking about the longer-term picture. There's no reason @barucden should have to suffer through all this. |
I don't wish to put words in their mouths, but I suspect that from among that list at least But anyway, you are right that the decision has been made and further back-and-forth probably won't be useful. Even though we don't agree in the end, I do appreciate that you took the issue seriously and took the time to respond in depth to raised concerns. |
Barring breaking changes (hence my initial suggestion during review to use a deprecation instead), my position continues to be that truncation is better than rounding. I think I've written that twice in this PR alone, and likely mentioned it indirectly on discourse. Regardless, I can see that this won't change your mind, just as lots of people on discourse don't, so this will be the last of me on this PR. |
I do know that some people have expressed a preference for truncation over rounding, and I'm genuinely sorry if you're disappointed by my decision here. But contrary to some of the expressed sentiments, this narrow issue is unaddressed by most of the discussion on discourse: a few people who have chimed in (mostly or exclusively here) on a specific technical issue and many more on the broader problems with Dates. We've gone over this before, and presumably there is no point in rehashing it again, but to try one more time: by my reading the technical arguments for truncation are not as unambiguous as they are for rounding. Terms like "simple mental model" and "worse to increase than decrease" seem subjective and thus open to debate. In contrast, reducing the rate of error accumulation is not subjective; you can argue whether that metric has any utility ("is that what we really should be minimizing here?"), and indeed such debates can be extremely important. There are indeed cases where one can come up with two objectively-good things and have them not be simultaneously achievable (e.g., https://en.wikipedia.org/wiki/Arrow%27s_impossibility_theorem), and in that case the discussion has to turn about which of these objective goals has greater value, placing it back in the realm of the subjective. But this is not such a case. So far, the only objective criterion that has been advanced in this discussion strongly favors rounding. It's quite hard to argue that a subjective preference should overrule an objective criterion, as subjective preferences are well-documented to depend strongly on past experience and two people coming to the same table may have very different past experiences. If someone proposes an objective criterion that obviously favors rounding, it would be very appropriate to reconsider this decision. This decision is also consistent with what Julia does elsewhere. When we compare |
I always fly merged airways 🍭. |
I'm quite unhappy about the course of this PR and do support the previous behaviour of truncating (best: error or depwarn). Date times represent starting points of intervals not single points in time. Let's say it's the last day of January and you are asked for the current month you would not round up and say "February", right? |
In German, if it's 6:30 you say "halb sieben," which translates as "half before 7." This is why I say such views are subjective, not objective. Nevertheless, the interval-perspective might be the most interesting countervailing argument anyone has raised yet. Deserves thought. The thing I hesitate about is that this perspective suggests some element of rigorous analysis. Truncation can guarantee the left edge but not the right edge; you'd be wrong to say that |
Thinking about this a bit more, the bottom line is there is no good solution with what we have now. I'd urge anyone who is sufficiently unhappy with the outcome here to just go ahead and implement That still won't fix |
Somewhen in the archives I discussed the logical necessity of treating
temporal intervals as [start, stop). I do not have the refs at hand --
they exist.
And with respect to PrecisionDateTime, anyone is welcome to use or to
borrow from either of these two approaches:
https://github.com/JuliaTime/NanoDates.jl or
https://github.com/JeffreySarnoff/TimesDates.jl.
…On Wed, Aug 23, 2023 at 7:03 AM Tim Holy ***@***.***> wrote:
Thinking about this a bit more, the bottom line is there is no good
solution with what we have now. I'd urge anyone who is sufficiently unhappy
with the outcome here to just go ahead and implement PrecisionDateTime
that's 128-bit and submit a PR. My reading of the discourse thread is that
we can't make DateTime 128-bit by default---the costs in data size are
too large. But we can have a PrecisionDateTime, and we can promote +(::DateTime,
::Union{Microsecond,Nanosecond}) to return a PrecisionDateTime. I think
that's non-breaking as long as PrecisionDateTime behaves the same way as
DateTime; it's a bit of a gray area in terms of whether changing return
type is allowed, but we tend to say "types are an implementation detail"
and it's behavior that we're protected. The fact that we're already pretty
borked here may also justify it essentially as a bugfix.
That still won't fix range(now()-Year(5), stop=now(), length=1001), but
that can be treated separately.
—
Reply to this email directly, view it on GitHub
<#50816 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAM2VRSXMO4QQJQAH4UFMV3XWXPOTANCNFSM6AAAAAA3GZCW6Y>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Resolves #50785EDIT: the PR changed over time, see the discussionThis is a proposal for fixing the linked issue. The PR consists of two commits.
The first commit just adds a few tests and one sentence to the docs of
DateTime
. I think this could be merged.The second commit changes addition/subtraction of a
DateTime
andMicrosecond
/Nanosecond
.Before:
After:
The second commit is breaking so I am not sure if it can be merged.
cc @jariji