Discussion thread for resolution of weeks rounding bug

[ptomato]

Normative PR #2722 resolved a bug with rounding durations to a largestUnit or smallestUnit of weeks. In the champions meeting of 2023-11-16 we agreed to discuss after merging and either affirm that this was the correct fix, or adjust it in a follow-up.

In the meeting, @gibson042 identified 3 interesting cases:

1. Nonzero years and/or months + large number of weeks. An example of this kind of duration is: 1 month 9 weeks 15 days (P1M9W15D).
2. Nonzero years and/or months + zero weeks. Example: 3 months 12 days (P3M12D).
3. Zero years + zero months + large number of weeks. Example: 52 weeks 20 days (P52W20D).

I've done some analysis of these three cases and will summarize it in this thread.

[ptomato]

First of all, here are links to the prior discussions I've been able to find on this issue.

• Thread starting here. Note that at that point, we hadn't yet developed the principle of largestUnit defaulting to the largest nonzero unit in the duration, and round() had an overflow: 'balance' or overflow: 'constrain' option which we later removed.
• Note this comment and this comment in that same thread, where we explicitly talked about the now-fixed "buggy" behaviour as intentional! However, what I did not realize at the time was that implementing it in that way essentially amounted to adding a duration to a date in non-RFC5545 order, which ought to be a non-starter.
• This comment claims that we never convert across year/week and month/week boundaries. I could be wrong in that comment, though.

What I can distill from the archive dive is these invariants:

1. If weeks is nonzero in the input of round then weeks will be filled in the output if needed.
2. If weeks is zero in the input then weeks will only be filled in the output if the user opts in via setting largestUnit: 'weeks' or smallestUnit: 'weeks'.

So, I think invariant (1) was buggy, and we have now fixed it. (Buggy because it essentially boiled down to doing date+duration addition in the order years, months, days, weeks - wrong according to RFC5545/iCalendar. It doesn't matter in the ISO calendar where weeks are always 7 days, but does matter very much in any calendar where week length can vary, which is a case that we do accommodate.)

That leaves (2) as the invariant that we need to uphold going forward.

There's a third invariant that's maybe overly obvious:

3. The output of round must contain no nonzero units smaller than smallestUnit, or larger than largestUnit.

[ptomato]

Next, here are the current rounding results for each of the 3 cases that Richard identified, with various largestUnits and smallestUnits.

In each test I've used a relativeTo of 2023-06-01 and a roundingMode of floor. (I didn't use halfExpand because I find consistenly rounding up or down slightly less confusing when thinking about these cases. It also comes in handy later when discussing alternative algorithms that double-round.)

Case 1: "months + big weeks" P1M9W15D

largestUnit → smallestUnit ↓	years	months	weeks
years	PT0S	-	-
months	P3M	P3M	-
weeks	P1M11W	P1M11W	P15W
none	P3M16D*	P3M16D*	P15W3D

*Before bugfix in #2722, this would have been P1M9W15D, unchanged from the input. All other results remain the same.

Case 2: "months + zero weeks" P3M12D

largestUnit → smallestUnit ↓	years	months	weeks
years	PT0S	-	-
months	P3M	P3M	-
weeks	P3M1W	P3M1W	P14W
none	P3M12D	P3M12D	P14W6D

The bugfix in #2722 didn't change any results in this table.

Case 3: "zero years + zero months + big weeks" P52W20D

largestUnit → smallestUnit ↓	years	months	weeks
years	P1Y	-	-
months	P1Y	P12M	-
weeks	P54W	P54W	P54W
none	P1Y18D*	P12M18D*	P54W6D

*Before bugfix in #2722, this would have been P52W20D, unchanged from the input. All other results remain the same.

Created with this script

/* eslint-disable no-console */
import * as Temporal from './lib/temporal.mjs';

function formatRounded(d, largestUnit, smallestUnit) {
  const relativeTo = Temporal.PlainDate.from('2023-06-01');
  const roundingMode = 'floor';
  const options = { relativeTo, roundingMode };
  return d
    .round({ ...options, largestUnit, smallestUnit })
    .toString()
    .padEnd(8);
}

function buildTable(d) {
  const yy = formatRounded(d, 'years', 'years');
  const ym = formatRounded(d, 'years', 'months');
  const yw = formatRounded(d, 'years', 'weeks');
  const yn = formatRounded(d, 'years', undefined);
  const mm = formatRounded(d, 'months', 'months');
  const mw = formatRounded(d, 'months', 'weeks');
  const mn = formatRounded(d, 'months', undefined);
  const ww = formatRounded(d, 'weeks', 'weeks');
  const wn = formatRounded(d, 'weeks', undefined);
  console.log(`\
| largest → smallest ↓ | years    | months   | weeks    |
| -------------------- | -------- | -------- | -------- |
|                years | ${yy   } | -        | -        |
|               months | ${ym   } | ${mm   } | -        |
|                weeks | ${yw   } | ${mw   } | ${ww   } |
|                 none | ${yn   } | ${mn   } | ${wn   } |
`);
}

console.log('\n=== Case 1: months + big weeks ===');
const d1 = Temporal.Duration.from({ months: 1, weeks: 9, days: 15 });
buildTable(d1);

console.log('\n== Case 2: months + zero weeks ===');
const d2 = Temporal.Duration.from({ months: 3, days: 12 });
buildTable(d2);

console.log('\n=== Case 3: zero years + zero months + big weeks ===');
const d3 = Temporal.Duration.from({ weeks: 52, days: 20 });
buildTable(d3);

Question

Do you see any results in this table that you find weird? Which ones?

[ptomato]

Next, a deeper dive on the current algorithm. Rounding the calendar units part of a duration essentially takes three steps. (Skip the parts marked "Details" unless you really want to read them. They are simplified prose descriptions of the spec algorithms.)

1. Balance the calendar units down until none are larger than largestUnit, leaving a potentially unbalanced duration. This is always a no-op with the default largestUnit. (UnbalanceDateDurationRelative)
   • Details: UnbalanceDateDurationRelative transforms { y, m, w, d } into:
     • no-op if largestUnit is year
     • { 0, m + yearsInMonths, w, d } if largestUnit is month
     • { 0, 0, w, d + yearsMonthsInDays } if largestUnit is week
     • { 0, 0, 0, d + yearsMonthsWeeksInDays } otherwise
2. Round the duration to the desired smallestUnit. (RoundDuration)
3. Balance the calendar units up to largestUnit if any are unbalanced. Because balancing weeks up to months may result in a remainder of days, we have two special cases when smallestUnit is weeks. (BalanceDateDurationRelative)
   • Details: BalanceDateDurationRelative, given input of { y, m, w, d }, has these invariants because it happens after RoundDuration:
     • If largestUnit is month, y = 0
     • If largestUnit is week, y = 0 and m = 0
     • If smallestUnit is year, m = 0, w = 0, and d = 0
     • If smallestUnit is month, w = 0 and d = 0
     • If smallestUnit is week, d = 0
   • It does the following transforms on the input:
     • If largestUnit is year, and smallestUnit is not week, balance all units up as much as possible and leave w = 0 in the output.
     • If largestUnit is year, and smallestUnit is week, balance months up into years, and leave weeks unchanged. (This is the first special-case for weeks.)
     • If largestUnit is month, and smallestUnit is not week, balance all units up into months and leave y = 0 and w = 0 in the output.
     • If largestUnit is month, and smallestUnit is week, no-op because y = 0 and d = 0 already. (This is the second special-case for weeks.)
     • If largestUnit is week, balance days up into weeks, and leave y = 0 and m = 0 in the output.
     • For other largestUnit (i.e., day or lower), no-op.

Now I want to take a look at how this works in practice for an example duration and an example rounding operation for each of the three cases that Richard identified. These are the same example durations as in the tables above.

Case 1: "months + big weeks" P1M9W15D rounded to smallestUnit weeks

d = Temporal.Duration.from({ months: 1, weeks: 9, days: 15 });
d.round({ smallestUnit: 'weeks', relativeTo: '2023-06-01', roundingMode: 'floor' })

• UnbalanceDateDurationRelative: P1M9W15D → default largestUnit, so no-op
• RoundDuration: P1M9W15D → weeks = 9+2=11, days = 1 → P1M11W (rounds down by 1 day)
• BalanceDateDurationRelative: P1M11W → no-op due to special case
• result = P1M11W, relativeTo + result = 2023-09-16, total rounding -1 day

Case 2: "months + zero weeks" P3M12D rounded to smallestUnit weeks

d = Temporal.Duration.from({ months: 3, days: 12 });
d.round({ smallestUnit: 'weeks', relativeTo: '2023-06-01', roundingMode: 'floor' })

• UnbalanceDateDurationRelative: P3M12D → default largestUnit, so no-op
• RoundDuration: P3M12D → P3M1W (rounds down by 5 days)
• BalanceDateDurationRelative: P3M1W → P3M1W (returns months and weeks directly due to special case)
• result: P3M1W, relativeTo + result = 2023-09-08, total rounding -5 days

Case 3: "zero years + zero months + big weeks" P52W20D rounded to largestUnit years, smallestUnit weeks

d = Temporal.Duration.from({ weeks: 52, days: 20 });
d.round({ smallestUnit: 'weeks', largestUnit: 'years', relativeTo: '2023-06-01', roundingMode: 'floor' })

• UnbalanceDateDurationRelative: P52W20D → largestUnit years, so no-op
• RoundDuration: P52W20D → P54W (rounds down by 6 days)
• BalanceDateDurationRelative: P54W → no months to balance into years → P54W
• result = P54W, relativeTo + result = 2024-06-13, total rounding -6 days

[ptomato]

Lots of exploration to report here! Next thing I'll talk about is alternative algorithms for rounding to weeks. Here's the one that was discussed during the champions meeting:

1. Balance everything down to days and then balance days up to weeks
2. Round to week, according to roundingMode
3. Balance back up to months or years
4. That will usually result in a remainder of days, so round to weeks again

I don't think we should do this "double rounding". For each of the 3 example durations from the previous comment, I'll show how it produces results which I think are worse than the status quo:

Case 1: "months + big weeks" P1M9W15D rounded to smallestUnit weeks

• Balance everything down to days and then up to weeks: P1M9W15D → P108D → P15W3D
• Round down to the nearest week: P15W3D → P15W (rounds down by 3 days)
• Balance everything up to months and then convert days to weeks: P15W → P3M13D → P3M1W6D
• Result is no longer rounded, so you need to round down again: P3M1W6D → P3M1W
• result = P3M1W, relativeTo + result = 2023-09-08, total rounding -9 days (abs should never be > 6)

Case 2: "months + zero weeks" P3M12D rounded to smallestUnit weeks

• Balance everything down to days and then up to weeks: P3M12D → P104D → P14W6D
• Round down to the nearest week: P14W6D → P14W (rounds down by 6 days)
• Balance everything up to months and then convert days to weeks: P14W → P3M6D
• Result is no longer rounded, so you need to round down again: P3M6D → P3M
• result = P3M, relativeTo + result = 2023-09-01, total rounding -12 days (abs should never be > 6)

Case 3: "zero years + zero months + big weeks" P52W20D rounded to largestUnit years, smallestUnit weeks

• Balance everything down to days and then up to weeks: P52W20D → P384D → P54W6D
• Round down to the nearest week: P54W6D → P54W
• Balance everything up to years and then convert to weeks: P54W → P1Y12D → P1Y1W5D
• Result is no longer rounded, so you need to round down again: P1Y1W5D → P1Y1W
• result = P1Y1W, relativeTo + result = 2024-06-08, total rounding -11 days (abs should never be > 6)

In each of these cases, converting the weeks in the post-rounding balance step results in a remainder of days, which needs to be rounded again. Normally I'd say rounding to the nearest week up or down should give you a duration that's no more than 6 days longer or shorter than the input duration, but here you can get up to 12 days!

(This is why I'm illustrating the algorithm with roundingMode: 'floor'. If the double-rounds might be in opposite directions, like with halfExpand, the discrepancies will be smaller and will average out. If the double-rounds are always in the same direction, the problem is much more serious.

This is even worse if roundingIncrement > 1. If you apply the rounding increment when you round both times, the discrepancies get larger. For example with smallestUnit: 'weeks', roundingIncrement: 2, roundingMode: 'floor' you'd expect that your duration would decrease by no more than 13 days, but with double-rounding it could decrease by almost a month.

Other alternatives

I tried to think of some other alternatives, but I concluded that any algorithm that balances weeks post-rounding is going to have the same problem. For example, this one would avoid balancing all the calendar units down in the first step:

1. Balance weeks down to days, then days up to months or years if largestUnit is months or years, then remaining days up to weeks
2. Round to week, according to roundingMode
3. Balance back up to months or years

This algorithm happens to work well on the duration in Case 1, for example, but you can still get a remainder of days in step 3 and therefore double-rounding. For example, if the input duration is P1M9W28D and roundingMode is ceil:

• Balance P1M9W28D → P3M29D → P3M4W1D
• Round up to the nearest week: P3M4W1D → P3M5W (rounds up by 6 days)
• Balance P3M5W → P4M5D
• Result is no longer rounded, so you need to round up again: P4M5D → P4M1W
• total rounding 8 days (should never be > 6)

Question

Any ideas for alternative algorithms that don't double-round?

[DiamondIceNS]

Hi there, spectator here. I am just a member of the peanut gallery who doesn't have the full picture of this project. But I've been fascinated by this problem, and, if I may, I would like to propose an alternative algorithm for your consideration. It is a refinement of the flawed algorithm proposed at the end of your analysis above:

1. Balance weeks down to days, then days up to months and/or years if largestUnit is months or years, leaving a remainder of days, d.
2. If days == 0, done. (No rounding needed)
3. Balance days up to weeks.
4. If days == 0, done. (No rounding needed)
5. Compute the total number of days contained within duration of the next available month beyond the ones tallied in Step 1, d_m.
6. Compute the total number of weeks found in Step 3 plus 1, and then compute how many days are in that combined duration, d_w.
7. If d_m <= d_w, increment months by 1 and set weeks and days to 0. Else, increment weeks by 1 and set days to 0.
8. Balance months up to years, if applicable.

By definition of Step 1, d should always be strictly lesser than d_m. Likewise, by definition of Step 3, d should always be strictly lesser than d_w. That gives two targets for round-up procedures.

If d_m == d_w, that simply means we have a month with a duration that fits an integer number of weeks (e.g. February in a non-leap year). That in turn means those weeks can smoothly balance up to a full month with no loss of accuracy, so rounding up to the next month should take precedence in this case.

In all other cases, exactly one of d_m or d_w will be closer to d than the other. I propose the one that is closest should be the chosen target to round to.

If I am not mistaken, this algorithm should:

• Never permit a rounding error greater than 6 days
• Always minimize the rounding error
• Not double round
• Be robust for roundingIncrement > 1

Unless I'm missing something, I believe that satisfies all of your requirements.

I am unfortunately not familiar enough with your project to be able to discern whether this algorithm breaks any of your other invariants, nor am I bright enough to discern whether this algorithm is congruent to one of the already proposed algorithms. I just figured I'd offer up the algorithm that seemed obvious to me after not seeing it proposed here thus far, just in case it was missed.

[ptomato]

Thanks for writing that up! I'll take a look soon at how it might work in the context of Temporal.Duration.

[ptomato]

This discussion has effectively become obsolete by what we are proposing in #2758, namely that the following two lines should always be equivalent to each other:

past.until(past.add(duration), { largestUnit, smallestUnit, roundingMode, etc })
duration.round({ relativeTo: past, largestUnit, smallestUnit, roundingMode, etc })

So we will by definition always use the same algorithm as until/since already uses. This produces the following results for the cases that I investigated above:

Case 1: months + big weeks

largest → smallest ↓	years	months	weeks
years	PT0S	-	-
months	P3M	P3M	-
weeks	P3M2W	P3M2W	P15W
none	P3M16D	P3M16D	P15W3D

Case 2: months + zero weeks

largest → smallest ↓	years	months	weeks
years	PT0S	-	-
months	P3M	P3M	-
weeks	P3M1W	P3M1W	P14W
none	P3M12D	P3M12D	P14W6D

Case 3: zero years + zero months + big weeks

largest → smallest ↓	years	months	weeks
years	P1Y	-	-
months	P1Y	P12M	-
weeks	P1Y2W	P12M2W	P54W
none	P1Y18D	P12M18D	P54W6D

Note in case 1, P3M2W instead of P1M11W, and in case 3, P1Y2W or P12M2W instead of 54W. This seems to address the concerns that people had about results not seeming natural.

@DiamondIceNS Thanks so much for proposing the alternative algorithm. I'm sorry it took so long to follow up on it — I've been busy resolving other issues with this proposal.

[DiamondIceNS]

No concerns here. Bigger fish gotta fry first. 👍

You've been crushing it on the issues lately. I take it a lot of co-dependent issues are finally cascading? Keep up the amazing work, looking forward to Temporal being a native part of JavaScript soon!