moment / luxon

ā± A library for working with dates and times in JS
https://moment.github.io/luxon
MIT License
15.23k stars 730 forks source link

Duration#rescale gives incorrect result #1457

Open Rivosoa opened 1 year ago

Rivosoa commented 1 year ago

Describe the bug When calling the method Duration#rescale, it gives incorrect result.

To Reproduce This is the code in which I discovered the error. Create a Duration object like this

let myDuration = Duration.fromObject({days: 364});

Log the result of the #rescale method

console.log(myDuration.rescale().toObject());

Actual vs Expected behavior Expected: It should log {months: 11, days: 30} (Because it's one year minus one day, or maybe there should be a week unit?)

Actual: The result is {years: 1, days: 4} (This causes wrong calculation in my app)

Desktop (please complete the following information):

Additional context I played with it a bit, and the way I understand the problem is that with a Duration object:

So if I create a Duration object with Duration.fromObject({days: 365});, Duration.fromObject({days: 360}); or Duration.fromObject({days: 358}); and I log console.log(myDuration.rescale().toObject());, all three duration will give the result {years: 1}, which result in the above error

icambron commented 1 year ago

Yeah, this is mostly the problem detailed here. So you might consider using longterm.

I'd have to dig back through to remember exactly how the algorithm works, but conceivably we could make the casual accuracy work a little better than giving { years: 1, days: 4 } by preventing rescale from recursively applying itself. But that would result in something more like 12 months, 4 days because 12 * 30 + 4 = 364. It's hard to see how it's ever going to get to 11 months, 30 days. That duration makes intuitive sense to a human because that close to the 1-year mark we start thinking of the units of time as relative to the year boundary and doing the math backward, which is what you're doing when you say "it's one year minus one day". But that isn't a systematically correct approach that can be easily codified in Luxon. Months are all different lengths, so how long is a month in abstract? If 11 months + 30 days is 364 days, then each month is 30.37 days, but that is not equal to 365/12, nor does it add up usefully for any other month/day combination. Instead of using the unit sizes consistently, we'd need a heuristic that let us change measure modes near certain size boundaries. But is 265 days a year minus 100 days or 0 plus 265 days? Where does it "flip" from being how-far-from-zero to how-far-from-next-year? And what happens at the discontinuity? In other words, the problem isn't really with Luxon, it's how we humans are fuzzy and heuristic with time periods. I'm very hesitant to start introducing those heuristics to the code, even if the way Luxon does it is unintuitive. That's because there are a zillion judgement calls, and it's hard to be concrete about what is "correct". Even when Luxon results look silly, as they do here, at least they're systematic.

Finally, depending on your use case, you might be interested in using Intervals to track specific periods of time on the calendar.

janvorwerk commented 8 months ago

IMHO, there should be a big warning and a link to the math doc on Duration API reference.

I have just been bitten by the same issue as shown in the following code extract:

  const start = DateTime.fromObject({ year: 2023, month: 1, day: 1, hour: 0, minute: 0, second: 0, millisecond: 0 });
  const end = DateTime.fromObject({ year: 2024, month: 1, day: 1, hour: 0, minute: 0, second: 0, millisecond: 0 });
  console.log("casual", end.diff(start, "hours").rescale().toObject());
  //   casual { years: 1, months: 1, days: 1 } āŒ
  console.log("longterm", end.diff(start, "hours", { conversionAccuracy: "longterm" }).rescale().toObject());
  //   longterm {
  //     months: 11,
  //     weeks: 4,
  //     days: 2,
  //     hours: 4,
  //     minutes: 39,
  //     seconds: 53,
  //     milliseconds: 999.9999963902155
  //   } šŸ™€

While Interval works much better:

  const start = DateTime.fromObject({ year: 2023, month: 1, day: 1, hour: 0, minute: 0, second: 0, millisecond: 0 });
  const end = DateTime.fromObject({ year: 2024, month: 1, day: 1, hour: 0, minute: 0, second: 0, millisecond: 0 });
  console.log("days interval", Interval.fromDateTimes(start, end).length("days"));
  // days interval 365 āœ…
  console.log("years interval", Interval.fromDateTimes(start, end).length("years"));
  // years interval 1 āœ