Wrong diff calculation

[VaheAntonyan]

Wrong diff calculation

To Reproduce

DateTime.fromISO("2021-04-01").diff(DateTime.fromISO("2020-02-29"), ["years", "months", "days"]).toObject()

Actual vs Expected behavior
Actual behavior: //=> {years: 1, months: 1, days: 4}
Expected behavior: //=> {years: 1, months: 1, days: 3}

• Luxon version 1.28.0, 3.0.4

[icambron]

This is a bug of sorts, but it's a subtle one. At some level it's a question of exactly what diffing across months and years really means. I certainly understand the intuition behind the 3 days: namely that 2021-04-01 is 3 days aster 2021-03-29, and it seems reasonable to interpret 03-29 and as a 1 year, 1 month after 02-29.

However, that last part isn't how Luxon does the diff. It adds the years first, then the months, then the days, figuring out at each step how much it has to add. And the problem is the year part: 2020-02-29 + 1 year isn't 2021-02-29 because that day doesn't exist. Instead it's 2021-02-28. A month after that it's 2021-03-28, and the 4 days after that it's 2021-04-01.

DateTime.fromISO("2020-02-29").plus({ years: 1 }).plus({ months: 1 }).plus({ days: 4 }).toISO()                                                                                   
//=> '2021-04-01T00:00:00.000-04:00'   

You could argue that Luxon should sort of combine the year and month calculations so that it doesn't decide the day in the new year until after it's figured out the months part, instead of deciding right after adding the year that the day is X and then continuing on. And there's precedence for that in Luxon. For example, if we do the add all at once instead of piecemeal as above, Luxon does exactly that:

DateTime.fromISO("2020-02-29").plus({ years: 1, months: 1, days: 4 }).toISO()                                                                                                     
//=> '2021-04-02T00:00:00.000-04:00' 

And I do think that whatever conventions Luxon uses, we should be consistent between what diff produces and what plus computes. In other words:

const someDiff = dt1.diff(dt2)
const addedBack = dt1.plus(someDiff)
addedBack.equals(dt1) // => should be true

That's being violated here, so we should fix diff to be consistent. So I'd take a PR for this as long as it wasn't like a ton of code.

[VaheAntonyan]

And I do think that whatever conventions Luxon uses, we should be consistent between what diff produces and what plus computes. In other words:

const someDiff = dt1.diff(dt2)
const addedBack = dt1.plus(someDiff)
addedBack.equals(dt1) // => should be true

That's being violated here, so we should fix diff to be consistent. So I'd take a PR for this as long as it wasn't like a ton of code.

const someDiff = dt1.diff(dt2)
const addedBack = dt2.plus(someDiff) // typo, dt2 instead of dt1
addedBack.equals(dt1) // => should be true

Actually diff and plus are consistent in this case.

I think that

dt1.plus({ years: x, months: y, days: z })

should be same, as

dt1.plus({ years: x }).plus({ months: y }).plus({ days: z })

but it is more like

dt1.plus({ months: y }).plus({ years: x }).plus({ days: z })

Consistency of going from bigger unit to smaller one is lost here, like we have for months and days case.

dt1.plus({ months: x, days: y })

dt1.plus({ months: x }).plus({ days: y })

[icambron]

Actually diff and plus are consistent in this case.

They aren't:

DateTime.fromISO("2020-02-29").plus({ years: 1, months: 1, days: 3 }).toISO()
//=> '2021-04-01T00:00:00.000-04:00'

DateTime.fromISO("2021-04-01").diff(DateTime.fromISO("2020-02-29"), ["years", "months", "days"]).toObject()
//=> {years: 1, months: 1, days: 4}

So 3 days vs 4 days, same spans of time. (If you add 4 days instead, you get April 2.) I think that's the core of the issue here.

I think that...should be same, as

I don't think that's right, and would violate a lot of expectations. Moreover, it would violate your expectations. Your original issue is that you expect your diff to give you 3 days, not 4. That's expecting Luxon to handle end-of-month logic first and then add the years, which is consistent with the current behavior of plus() but not with the current behavior of diff(). Doing years then months then days is exactly what diff() does, which runs into "2021-02-29 doesn't exist" problems.

Or put another way, this is consistent with diff and apparently not what you want:

DateTime.fromISO("2020-02-29").plus({ years: 1}).plus({ months: 1}).plus({ days: 3 }).toISO() // => '2021-03-31T00:00:00.000-04:00'

I agree with original you and disagree with new you.

[VaheAntonyan]

Yes, you are right, my 2nd comment is wrong, thank you.

[dobon]

Here's a test case for this:

// see https://github.com/moment/luxon/issues/1301
test("DateTime#diff handles Feb-29 edge case logic for higher order units in a manner consistent with DateTime#plus", () => {
  const left = DateTime.fromISO("2020-02-29");
  const right = DateTime.fromISO("2021-04-01");

  const diff = right.diff(left, ["year", "month", "day"]);
  expect(diff.days).toBe(3);
  expect(left.plus(diff).equals(right)).toBe(true);
});

I haven't really wrapped my head around the exact nature of this bug and the current diff algorithm properly yet, but I notice that these dates also span DST and wonder if that implied change in offset may be playing a role as well. I am finding it challenging to generalize this bug with other dates that are problematic in a similar way, but I will try to puzzle it out and will post a PR if I am successful.

[icambron]

@dobon thanks for the help here.

It's not a very general bug. The best way to think of it is, "what is 2020-02-29 plus 1 year, 1 month"?

• If you add the year first you land on 2021-02-29, which doesn't exist, so you change that to 2021-02-28, and then you add a month, resulting in 2021-02-28. .plus({years: 1}).plus({ months: 1}) it comes up with 03-28 where you expect 03-29
• If you consider months and years more carefully, you can sort of "delay" the day computation until you land on the right month and then set it. That gets you 2022-03-29. This is what .plus({years: 1, months: 1}) does, though it uses a sort of oblique technique to pull that off

In other words, it's very specific to leap years and ends of months. I don't think DST plays any part.

The way diff works is that it starts adding units one at a time. It takes the units in order and just adds as much of that unit as it can. So like .diff(..., ["years", "months", "days"]) adds as many years as it can, then as many months as it can, then as many days as it can (it does the "as much as it can" part with a pretty naive subtraction called "differ" in the code, then subtracts one back off if it overshoots). There are complications there having to do with what you do with remainders, but that's the gist of it.

So the problem is really that it does it one unit at a time, and those are complete additions, as in literal plus({ /* one unit and value */ }) calls. So it's just like the first method of addition above, and it never gets a chance to do the second method.

Edit: it's worth looking at how plus() actually does this. It's not particularly clever, but it might lead to the right technique to use in diff. I think what diff needs to do is probably something like this:

1. keep track of the high-water calendar units individually, instead of just a high-water datetime
2. at each step, only construct datetimes out of that for comparison purposes (i.e. to see if we exceeded the target date), then we throw it away. This might be a performance problem

That way, if we accumulated 1 year, we think the high water is { year: 2021, month: 02, day: 29 } which is invalid, but we don't do anything about it, just keep going. That should let us get to 03-29. But I haven't actually tried it and it might be more complicated than that.

[dobon]

@icambron Here's my PR: #1340

The key change I made was to avoid using an intermediate "cursor" DateTime, and instead build up a duration-like hash which is always directly added to the 'earlier' DateTime and compared against the 'later'. This way we are directly using the hash-based DateTime#plus method when creating the diff Duration, thus it is guaranteed to be consistent.

Please let me know what you think about this solution, when you have time to review.

[dobon]

@icambron I've cleaned up my PR so that there are now very simple and minimal changes from current algorithm. Actually, I'm surprised how clean the patch turned out, lol. It should be easy for you to review now. Cheers.

[dobon]

This issue can be closed now. Fix was published in 3.2.0