2.3 hour duration gives 2h 17m (should be 2h 18m)

[afanasy]

//moment.js 2.11.2
moment.duration(0.3, 'h').minutes()
//18, as expected
moment.duration(2.3, 'h').minutes()
//17
//?!?

I understand that it's floating point problem, but can we work around this somehow, by not converting everything to milliseconds for instance?

[icambron]

Yeah, that seems like a bug. Caused by this, I assume:

> moment.duration(2.3, 'h').asMilliseconds()
8279999.999999999

[mattjohnsonpint]

Oh JavaScript floating point numbers, you're so funny...

2.3 * 60 * 60 * 1000 === 8280000
60 * 60 * 1000 === 3600000
2.3 * 3600000 === 8279999.999999999

WAT.

[mattjohnsonpint]

Actually I think this one may require some more detailed thought in how we apply large constants against multiplication and division. Probably not ideal for up-for-grabs, but if you think you have a good approach, still send a PR. Be sure to include a unit test with at least the above example.

[afanasy]

@mj1856 Initially the approach I'd take is to store duration values with units and use them directly for conversions in .units() calls, w/o converting to ms.

//2.3 hours to minutes
//current 2.11.2 version
2.3 * 3600000 / 60000 === 137.99999999999997 //minutes - wrong
//proposal
2.3 * 60 === 138 //minutes - correct

but now I don't know, may be it will produce new errors with some other floating point numbers, is it really the problem with division and/or large constants?

[mattjohnsonpint]

Note how there was no division in my examples.

[afanasy]

Then may be we should use some lib (or corresponding code subset) like https://github.com/infusion/Fraction.js for this?

[icambron]

Eh, floating point numbers are just painful in general. FWIW, Java behaves exactly like Matt's "WAT" example.

I propose we just change this line to

hours * 3600 * 1000

Does that suffice?

[mattjohnsonpint]

Works for me. We should have a test for that as well, as I could see someone accidently putting it back during a future cleanup.

Are there other areas like this?

[mnt]

Sorry I might be wrong but while playing around:
actual result:

4.1 * 3600 * 1000 === 14759999.999999998
4.17123 * 1000 * 60 * 60 === 15016428.000000002

expected result:

4.1 * 3600 * 1000 === 14760000
4.17123 * 1000 * 60 * 60 === 15016428

Perhaps a way to fix this is to make all floating point numbers to integers. Then dividing by the multiplier afterwards ?

(4.1 * 10 * 3600 * 1000) / 10 === 14760000
(4.17123 * 100000 * 1000 * 60 * 60) / 100000 === 15016428

[afanasy]

@mnt Looks interesting, but doesn't work for me for

(4.17123 * 100000 * 1000 * 60 * 60) / 100000
//outputs 15016428.000000002 in Google Chrome console

but still may be it covers more cases

[mnt]

oh im sorry you are right. I must have been too caught up.

[icambron]

Floating point numbers are the worst. Thinking about this a bit more, the real alternative is to just do some smarter rounding. Specifically, it's one thing to be off by 0.0000001, but it's another to round that down really far, as in the original issue, where we apparently rounded 17.999983333333333 down to 17.

[afanasy]

Yes, probably should be on this line https://github.com/moment/moment/blob/develop/src/lib/duration/get.js#L18

[arntj]

@afanasy This gives the exact number in Chrome console.

parseInt(4.17123 * 100000 * 1000 * 60 * 60) / 100000

(It's like mnt suggests above, multiplying and dividing by a big factor, but with an extra conversion to int).

Doing something like this (or simply rounding off the milliseconds according to conventional rules) would possibly solve this issue, but perhaps that is a sub optimal solution from a performance or precision perspective?

[afanasy]

@arntj yes, I think the @mnt suggestion - making milliseconds int out of float and then operating on it - is the best approach at the moment. While it probably will not fix all floating point issues, it will improve calculations for many similar cases. @mj1856 @icambron what do you think?

[marwahaha]

did this get finished?

[afanasy]

@marwahaha I'm not sure, because it's basically the floating point numbers standard behaviour. To fix this completely we will need to use some decimal lib to calculate things, like big.js internally.

[ashsearle]

Is there an unwritten commitment to supporting durations to sub-millisecond accuracy?

If there isn't, you could just round-off milliseconds in the Duration constructor:

this._milliseconds = absRound(+milliseconds +
    seconds * 1e3 + // 1000
    minutes * 6e4 + // 1000 * 60
    hours * 36e5 // 1000 * 60 * 60
);

[afanasy]

@ashsearle that's interesting. But I'm wondering if sub milliseconds could appear unintentionally as a by-product of some conversion 🤔

[marwahaha]

related to #4649