Time>addSeconds: ignores nanos ivar value
Locked
Time>addSeconds: ignores nanos ivar value
 
|
Time >addSeconds: completely ignores the nanos ivar value and so
Time now -> (say) 12:51:47.418205 pm send that #addSeconds: 2 -> 12:51:49 pm Which is incorrect. Actually *fixing* it seems to be a bit more complex than just "add the nanoseconds stuff". Should #asSeconds be truncating the seconds and ignoring the nanoseconds? That's also an issue for #addTime: and #subtractTime: (wait, no #subtractSeconds: ?) Chronology-afficonados assemble! tim -- tim Rowledge; [hidden email]; http://www.rowledge.org/tim Fractured Idiom:- PRO BOZO PUBLICO - Support your local clown |
Re: Time>addSeconds: ignores nanos ivar value
|
|
On Mon, Jul 20, 2020 at 01:08:57PM -0700, tim Rowledge wrote:
> Time >addSeconds: completely ignores the nanos ivar value and so > Time now -> (say) 12:51:47.418205 pm > send that #addSeconds: 2 -> 12:51:49 pm > > Which is incorrect. Actually *fixing* it seems to be a bit more complex than just "add the nanoseconds stuff". > > Should #asSeconds be truncating the seconds and ignoring the nanoseconds? That's also an issue for #addTime: and #subtractTime: (wait, no #subtractSeconds: ?) > > Chronology-afficonados assemble! > > tim It looks broken to me. And deserving of a unit test to document the expected result. In Squeak 3.8 #addSeconds: seems to do the same thing, but "Time now" gives an instance with nanos equal to zero, so possibly it is old behavior that was just not noticed. In DateAndTime, we introduced #asExactSeconds to allow backward compatibility with the original #asSeconds that answered whole seconds. Maybe something similar would be needed for Time. Or maybe it should just be this: Time class>>addSeconds: nSeconds "Answer a Time that is nSeconds after the receiver." ^ self class seconds:seconds + nSeconds nanoSeconds: nanos Either way a unit test is the first step. The existing TimeTest>>testAddSeconds does not provide any coverage for instances of Time with non-zero nanos. Dave |
Re: Time>addSeconds: ignores nanos ivar value
|
|
It isn't my field at all but since time is not really quantised until Plank-time scale, shouldn't we be using floating point for seconds? Or is the argument that keeping a couple of integers is 'cheaper'?
Also, should the added seconds be restricted to an integer number of seconds? What ought happen if a float is passed in? > On 2020-07-20, at 1:45 PM, David T. Lewis <[hidden email]> wrote: > > On Mon, Jul 20, 2020 at 01:08:57PM -0700, tim Rowledge wrote: >> Time >addSeconds: completely ignores the nanos ivar value and so >> Time now -> (say) 12:51:47.418205 pm >> send that #addSeconds: 2 -> 12:51:49 pm >> >> Which is incorrect. Actually *fixing* it seems to be a bit more complex than just "add the nanoseconds stuff". >> >> Should #asSeconds be truncating the seconds and ignoring the nanoseconds? That's also an issue for #addTime: and #subtractTime: (wait, no #subtractSeconds: ?) >> >> Chronology-afficonados assemble! >> >> tim > > It looks broken to me. And deserving of a unit test to document the expected result. > > In Squeak 3.8 #addSeconds: seems to do the same thing, but "Time now" gives > an instance with nanos equal to zero, so possibly it is old behavior that > was just not noticed. > > In DateAndTime, we introduced #asExactSeconds to allow backward compatibility > with the original #asSeconds that answered whole seconds. Maybe something > similar would be needed for Time. > > Or maybe it should just be this: > > Time class>>addSeconds: nSeconds > "Answer a Time that is nSeconds after the receiver." > > ^ self class > seconds:seconds + nSeconds > nanoSeconds: nanos > > > Either way a unit test is the first step. The existing TimeTest>>testAddSeconds > does not provide any coverage for instances of Time with non-zero nanos. > > Dave > > > tim -- tim Rowledge; [hidden email]; http://www.rowledge.org/tim Press [ESC] to detonate or any other key to explode. |
Re: Time>addSeconds: ignores nanos ivar value
|
|
No, the magnitude of time should not be a Float. Nor should it be an Integer.
It should be a Number. In practice, the numbers tend to be integer-ish in nature for one of two reasons: 1) The time value is derived from a system call to some computer operating system that reports time that way. DateAndTime now has integral instance variables for this reason. 2) The user is thinking of "time" as something that shows up on a Timex digital wristwatch display, or on a wind-up clock that makes loud ticking noises on integer boundaries. Time>>addSeconds: is an example of that mindset. Having said that, there is absolutely no reason that this should not work: DateAndTime fromSeconds: Float pi Which it does. And you will find that the magnitude (utcMicroseconds instance variable) is in fact a SmallFloat64 as it should be. But in practice Integer, Fraction, and ScaledDecimal are the must useful and common types of Number to use when representing a time magnitude. Dave On Mon, Jul 20, 2020 at 02:00:40PM -0700, tim Rowledge wrote: > It isn't my field at all but since time is not really quantised until Plank-time scale, shouldn't we be using floating point for seconds? Or is the argument that keeping a couple of integers is 'cheaper'? > > Also, should the added seconds be restricted to an integer number of seconds? What ought happen if a float is passed in? > > > On 2020-07-20, at 1:45 PM, David T. Lewis <[hidden email]> wrote: > > > > On Mon, Jul 20, 2020 at 01:08:57PM -0700, tim Rowledge wrote: > >> Time >addSeconds: completely ignores the nanos ivar value and so > >> Time now -> (say) 12:51:47.418205 pm > >> send that #addSeconds: 2 -> 12:51:49 pm > >> > >> Which is incorrect. Actually *fixing* it seems to be a bit more complex than just "add the nanoseconds stuff". > >> > >> Should #asSeconds be truncating the seconds and ignoring the nanoseconds? That's also an issue for #addTime: and #subtractTime: (wait, no #subtractSeconds: ?) > >> > >> Chronology-afficonados assemble! > >> > >> tim > > > > It looks broken to me. And deserving of a unit test to document the expected result. > > > > In Squeak 3.8 #addSeconds: seems to do the same thing, but "Time now" gives > > an instance with nanos equal to zero, so possibly it is old behavior that > > was just not noticed. > > > > In DateAndTime, we introduced #asExactSeconds to allow backward compatibility > > with the original #asSeconds that answered whole seconds. Maybe something > > similar would be needed for Time. > > > > Or maybe it should just be this: > > > > Time class>>addSeconds: nSeconds > > "Answer a Time that is nSeconds after the receiver." > > > > ^ self class > > seconds:seconds + nSeconds > > nanoSeconds: nanos > > > > > > Either way a unit test is the first step. The existing TimeTest>>testAddSeconds > > does not provide any coverage for instances of Time with non-zero nanos. > > > > Dave > > > > > > > > > tim > -- > tim Rowledge; [hidden email]; http://www.rowledge.org/tim > Press [ESC] to detonate or any other key to explode. > > > |
Re: Time>addSeconds: ignores nanos ivar value
|
|
In reply to this post by timrowledge
On Mon, Jul 20, 2020 at 01:08:57PM -0700, tim Rowledge wrote:
> Time >addSeconds: completely ignores the nanos ivar value and so > Time now -> (say) 12:51:47.418205 pm > send that #addSeconds: 2 -> 12:51:49 pm > > Which is incorrect. Actually *fixing* it seems to be a bit more complex > than just "add the nanoseconds stuff". > > Should #asSeconds be truncating the seconds and ignoring the nanoseconds? > That's also an issue for #addTime: and #subtractTime: (wait, no #subtractSeconds: ?) > > Chronology-afficonados assemble! > I'm a bit late following up on this, but I'm working on some unit tests and a fix to be submitted to inbox soon. I took a look at Squeak 3.6 (prior to the introduction of the Chronology package). I was expecting that Time instances had originally assumed the use of whole seconds, but to my pleasant surprise, it is not so. Here is what I see in Squeak 3.6, which looks entirely right to me (note that I am intentionally using a completely inappropriate Float value to illustrate the point): t1 := Time fromSeconds: Float pi. "==> 12:00:03.141592653589793 am" t2 := t1 addSeconds: Float pi. "==> 12:00:06.283185307179586 am" t1 seconds. "==> 3.141592653589793" t1 asSeconds. "==> 3.141592653589793" t2 seconds. "==> 6.283185307179586" t2 asSeconds. "==> .283185307179586" t2 asSeconds - t1 asSeconds. "==> 3.141592653589793" And here is what I see in trunk today, which is clearly broken: t1 := Time fromSeconds: Float pi. "==> 12:00:03.141592653 am" t2 := t1 addSeconds: Float pi. "==> 12:00:06.141592654 am" t1 seconds. "==> 3" t1 nanoSecond. "==> 141592653" t1 asSeconds. "==> 3" t1 asNanoSeconds. "==> 3141592653" t2 seconds. "==> 6" t2 nanoSecond. "==> 141592654" t2 asSeconds. "==> 6" t2 asNanoSeconds. "==> 6141592654" t2 asSeconds - t1 asSeconds. "==> 3" t2 asNanoSeconds - t1 asNanoSeconds. "==> 3000000001" By fixing #addSeconds to respect the nanos ivar, we will have something more like this: t1 := Time fromSeconds: Float pi. "==> 12:00:03.141592653 am" t2 := t1 addSeconds: Float pi. "==> 12:00:06.283185307 am" t1 seconds. "==> 3" t1 nanoSecond. "==> 141592653" t1 asSeconds. "==> 3" t1 asNanoSeconds. "==> 3141592653" t2 seconds. "==> 6" t2 nanoSecond. "==> 283185307" t2 asSeconds. "==> 6" t2 asNanoSeconds. "==> 6283185307" t2 asSeconds - t1 asSeconds. "==> 3" t2 asNanoSeconds - t1 asNanoSeconds. "==> 3141592654" That will still leave us with inconsistent behavior with respect to #asSeconds, but that is probably a separate topic, and best checked against the behavior of other Smalltalk dialects for compatibility. Dave |
Re: Time>addSeconds: ignores nanos ivar value
|
|
Out of the dozen or so lines of code listed, #asSeconds it looks like
the only one that's broken, it should be the number of seconds since Time's epoch. It must've been broken recently, it's still correct in 5.3. The other accessors without "as", like #seconds, are supposed to be the "clock readout" of those values, and _should_ never return a fractional value. Nothing in Chronology was ever intended to use or return fractional (e.g., Float or Fraction) values, only integers. If we still have #exactSeconds, we should probably delete it and introduce #fractionalSeconds if you feel you want that. Subseconds access was intended to be accessed via the explicit accessors (#nanoSecond and #asNanoSeconds). Introducing a new separate mechanism for precision that utilizes Fractions sounds like it would subvert the efficiency of your UTC upgrade, and be confusing to have in conjunction with the API that already provides access to the subsecond values. Chronology needs to be fast and efficient. To me, Tim's original example of #addSeconds: dropping the nanos seems like the only other bug, so far. - Chris On Sun, Sep 6, 2020 at 5:08 PM David T. Lewis <[hidden email]> wrote: > > On Mon, Jul 20, 2020 at 01:08:57PM -0700, tim Rowledge wrote: > > Time >addSeconds: completely ignores the nanos ivar value and so > > Time now -> (say) 12:51:47.418205 pm > > send that #addSeconds: 2 -> 12:51:49 pm > > > > Which is incorrect. Actually *fixing* it seems to be a bit more complex > > than just "add the nanoseconds stuff". > > > > Should #asSeconds be truncating the seconds and ignoring the nanoseconds? > > That's also an issue for #addTime: and #subtractTime: (wait, no #subtractSeconds: ?) > > > > Chronology-afficonados assemble! > > > > I'm a bit late following up on this, but I'm working on some unit tests > and a fix to be submitted to inbox soon. > > I took a look at Squeak 3.6 (prior to the introduction of the Chronology > package). I was expecting that Time instances had originally assumed the > use of whole seconds, but to my pleasant surprise, it is not so. > > Here is what I see in Squeak 3.6, which looks entirely right to me (note > that I am intentionally using a completely inappropriate Float value to > illustrate the point): > > t1 := Time fromSeconds: Float pi. "==> 12:00:03.141592653589793 am" > t2 := t1 addSeconds: Float pi. "==> 12:00:06.283185307179586 am" > t1 seconds. "==> 3.141592653589793" > t1 asSeconds. "==> 3.141592653589793" > t2 seconds. "==> 6.283185307179586" > t2 asSeconds. "==> .283185307179586" > t2 asSeconds - t1 asSeconds. "==> 3.141592653589793" > > And here is what I see in trunk today, which is clearly broken: > > t1 := Time fromSeconds: Float pi. "==> 12:00:03.141592653 am" > t2 := t1 addSeconds: Float pi. "==> 12:00:06.141592654 am" > t1 seconds. "==> 3" > t1 nanoSecond. "==> 141592653" > t1 asSeconds. "==> 3" > t1 asNanoSeconds. "==> 3141592653" > t2 seconds. "==> 6" > t2 nanoSecond. "==> 141592654" > t2 asSeconds. "==> 6" > t2 asNanoSeconds. "==> 6141592654" > t2 asSeconds - t1 asSeconds. "==> 3" > t2 asNanoSeconds - t1 asNanoSeconds. "==> 3000000001" > > By fixing #addSeconds to respect the nanos ivar, we will have > something more like this: > > t1 := Time fromSeconds: Float pi. "==> 12:00:03.141592653 am" > t2 := t1 addSeconds: Float pi. "==> 12:00:06.283185307 am" > t1 seconds. "==> 3" > t1 nanoSecond. "==> 141592653" > t1 asSeconds. "==> 3" > t1 asNanoSeconds. "==> 3141592653" > t2 seconds. "==> 6" > t2 nanoSecond. "==> 283185307" > t2 asSeconds. "==> 6" > t2 asNanoSeconds. "==> 6283185307" > t2 asSeconds - t1 asSeconds. "==> 3" > t2 asNanoSeconds - t1 asNanoSeconds. "==> 3141592654" > > That will still leave us with inconsistent behavior with respect to > #asSeconds, but that is probably a separate topic, and best checked > against the behavior of other Smalltalk dialects for compatibility. > > Dave > > |
Re: Time>addSeconds: ignores nanos ivar value
|
|
In reply to this post by timrowledge
On Mon, Jul 20, 2020 at 01:08:57PM -0700, tim Rowledge wrote:
> Time >addSeconds: completely ignores the nanos ivar value and so > Time now -> (say) 12:51:47.418205 pm > send that #addSeconds: 2 -> 12:51:49 pm > > Which is incorrect. Actually *fixing* it seems to be a bit more complex > than just "add the nanoseconds stuff". > > Should #asSeconds be truncating the seconds and ignoring the nanoseconds? > That's also an issue for #addTime: and #subtractTime: (wait, no #subtractSeconds: ?) > > Chronology-afficonados assemble! > > tim In the inbox now, test coverage for the addSeconds: issue is in Chronology-Tests-dtl.24 and a fix is in Chronology-Core-dtl.60. I have not looked at addTime: yet but it may require similar treatment. Dave |
«
Return to Squeak - Dev
|
1 view|%1 views
| Free forum by Nabble | Edit this page |