Luxon: Month diff is a little bit incorrect (exactly the same interval for two subsequent days)

[ilyazub]

Month interval is incorrectly calculated for two subsequent days.

When using Luxon v0.4.0 I get

2018-02-25 - 2017-09-25 = 5 months
2018-02-24 - 2017-09-25 = 5 months

Moment.js correctly handles this case.

// moment.js v2.20.1
[ '2018-02-23', '2018-02-24', '2018-02-25', '2018-02-26', '2018-03-24', '2018-03-25', '2018-03-26' ].map(date => moment(date).diff('2017-09-25', 'month', true))
// => [4.933333333333334, 4.966666666666667, 5, 5.032258064516129, 5.966666666666667, 6, 6.032258064516129]

// Luxon v0.4.0
[ '2018-02-23', '2018-02-24', '2018-02-25', '2018-02-26', '2018-03-24', '2018-03-25', '2018-03-26' ].map(date => Interval.fromDateTimes(DateTime.fromISO('2017-09-25'), DateTime.fromISO(date)).length('months'))
// => [4.966666666666667, 5, 5, 5.033333333333333, 5.9, 6, 6.031944444444444]

Seems to be relevant:
moment/moment#571
moment/luxon#167

[icambron]

So, I understand what's happening here, but I need to think a bit about how to fix it cleanly. When Luxon takes that diff, it realizes that not quite 5 months has past, so it knows it's 4 and some fraction. To compute that fractional part, it takes the remaining milliseconds (the time between 4 months after the start date and the end date) and rolls them up into months, then adds that to 4. It does that rolling up part naively, without regard to which months they are. Because the default conversion system has 1 month = 30 days, it says "my remainder is 1", adds it back to the much more carefully calculated 4, and ends up at the totally bogus 5.

What it actually needs to do is build up the fraction incrementally through each of the lower order units, so 30 out of 31 days, etc. There's almost certainly an equivalent bug for leap years and DST days, so this has to cascade through a bunch of units. I think the best thing to do is to calculate all the units preemptively (down to hours, anyway), and then "fractionalize" them at the end if they're not in the diff unit list.

Sounds complicated. I'll see if I can give it a shot soon.

[icambron]

Yeah, here are the test cases:

test('DateTime#diff handles fractional years as fractions of those specific years', () => {
  // the point here is that we're crossing the leap year
  expect(
    diffObjs({ year: 2020, month: 3, day: 27 }, { year: 2018, month: 3, day: 28 }, 'years')
  ).toEqual({ years: 1 + 365.0/366 });
});

test('DateTime#diff handles fractional months as fractions of those specific months', () => {
  // The point here is that January has 31 days
  expect(
    diffObjs({ year: 2018, month: 2, day: 24 }, { year: 2017, month: 12, day: 25 }, 'months')
  ).toEqual({ months: 1 + 30.0/31 });
});

test('DateTime#diff handles fractional weeks as fractions of those specific weeks', () => {
  // America/New_York has a fall back Nov 4, 2018 at 2:00
  expect(
    diffObjs({ year: 2018, month: 11, day: 16, hour: 0}, { year: 2018, month: 11, day: 2, hour: 1 }, 'weeks')
  ).toEqual({ weeks: 1 + 6.0/7 + 23.0/24/7 });
});

test('DateTime#diff handles fractional days as fractions of those specific days', () => {
  // America/New_York has a fall back Nov 4, 2018 at 2:00
  expect(
    diffObjs({ year: 2018, month: 11, day: 5, hour: 0}, { year: 2018, month: 11, day: 3, hour: 1 }, 'days')
  ).toEqual({ days: 1 + 23.0/24 });
});

The days and weeks ones were already handled by using the UTC days and those tests already pass. But the month one is the issue posted above and the year one is its leap-year equivalent, both of which currently fail. I have a fix in mind now, though.

[icambron]

Fixed in 07e8d5e. I haven't released it yet, but do you mind taking it for a spin?

[icambron]

Nevermind, released in 0.5.1

[ilyazub]

Now it's correct. @icambron, thank you!

[ '2018-02-23', '2018-02-24', '2018-02-25', '2018-02-26', '2018-03-24', '2018-03-25', '2018-03-26' ].map(date => Interval.fromDateTimes(DateTime.fromISO('2017-09-25'), DateTime.fromISO(date)).length('months'))

// => [4.935483870967742, 4.967741935483871, 5, 5.035714285714286, 5.964285714285714, 6, 6.0309555854643335]