Time>addSeconds: ignores nanos ivar value

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

Time>addSeconds: ignores nanos ivar value

timrowledge
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



Reply | Threaded
Open this post in threaded view
|

Re: Time>addSeconds: ignores nanos ivar value

David T. Lewis
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


Reply | Threaded
Open this post in threaded view
|

Re: Time>addSeconds: ignores nanos ivar value

timrowledge
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.



Reply | Threaded
Open this post in threaded view
|

Re: Time>addSeconds: ignores nanos ivar value

David T. Lewis
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.
>
>
>