More relativeTo rounding

[ptomato]

This fixes rounding and totalling Durations with a ZonedDateTime relativeTo parameter. There is still one bug in the algorithm, which is causing one rounding test to fail, and the corresponding totalling test, so those two remain skipped until I can track down the bug.

Closes: #1171

[codecov]

Codecov Report

Merging #1185 (27fa9cf) into main (5930868) will increase coverage by 1.18%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1185      +/-   ##
==========================================
+ Coverage   93.55%   94.73%   +1.18%     
==========================================
  Files          19       19              
  Lines        7961     9429    +1468     
  Branches     1264     1388     +124     
==========================================
+ Hits         7448     8933    +1485     
+ Misses        506      490      -16     
+ Partials        7        6       -1     

Flag	Coverage Δ
test262	37.49% <ø> (-4.12%)	⬇️
tests	90.91% <ø> (+1.47%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
polyfill/lib/legacydate.mjs
polyfill/lib/shim.mjs
polyfill/lib/intrinsicclass.mjs
polyfill/lib/slots.mjs
lib/plainmonthday.mjs	91.80% <0.00%> (ø)
lib/plaindatetime.mjs	95.59% <0.00%> (ø)
lib/plaindate.mjs	93.31% <0.00%> (ø)
lib/plaintime.mjs	93.82% <0.00%> (ø)
lib/calendar.mjs	82.98% <0.00%> (ø)
lib/temporal.mjs	100.00% <0.00%> (ø)
... and 13 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0f6fc83...90ca146. Read the comment docs.

[justingrant]

Hi @ptomato - In case it's useful, here's a test case from a "DST best practices" page for the docs that I'm working on. Both .map() calls should return the same results, but currently the one calling total returns 840 for both array elements, which is incorrect. In the docs I'm assuming that the bug will be fixed. ;-)

days = 35;

// WRONG!!! One calendar day during this period might not be 24 hours long!
hours = days * 24; // => 840

// CORRECT: interpret the duration in the time zone where it was measured
// One hour may have been skipped by a DST transition.
duration = Temporal.Duration.from({ days });
referenceDate = Temporal.PlainDate.from('2020-03-01');
tzTokyo = Temporal.TimeZone.from('Asia/Tokyo');
tzBerlin = Temporal.TimeZone.from('Europe/Berlin');

[hoursInTokyo, hoursInBerlin] = [tzTokyo, tzBerlin].map( 
  tz => duration.total({
    unit: 'hours',
    relativeTo: referenceDate.toZonedDateTime(tz)
  })
); // => [840, 839]

// ALSO CORRECT: Instead of using `total()` or `round()` on the 
// duration, you could add the duration to the reference date
// and then use `since` or `until`.
[hoursInTokyo, hoursInBerlin] = [tzTokyo, tzBerlin].map( 
  tz => referenceDate.toZonedDateTime(tz)
    .until(
      referenceDate.toZonedDateTime(tz).add(duration), 
      { largestUnit: 'hours' }
    )
    .hours
); // => [840, 839]

[ptomato]

@justingrant Thanks for the extra example! I finally figured out what the discrepancy was and fixed the tests, and now the example checks out as well.

[justingrant]

I think this looks good and I think I understand what it's doing, although the logic is complicated enough that it may be worth a 1:1 walkthrough if you have time. Your call.

I did have one concern about O(n) looping on years, but that seems like something that could be optimized later if needed. As an alternative, it's probably possible to use PlainDateTime math (aka Calendar.prototype.dateUntil) and then back up if if the ZDT result goes too far. This could delegate the O(n) problem to the calendar which might enable an ISO optimization.

[justingrant]

Is an O(n) loop necessary for years? If there's a 400K-year duration, that's a lot of looping! Although some calendars may have a variable number of years, ISO years (the vast majority of usage) are deterministic and this calculation could probably be O(1) for ISO. Looping for days/months seems OK because there's not going to be 1000s of them.

[ptomato]

I was also thinking over the weekend that a lot of this code could be skipped in many cases if we used getNextTransition/getPreviousTransition.

Would you be interested in doing the walkthrough and possibly identifying ways to optimize these algorithms next week? I'm not sure I can bring myself to look at them again this week. 😬

[justingrant]

Sure, would be happy to review next week! I spent today doing a deep dive on the new code and have two questions that may be bugs in the algorithm. Will make a separate comment about that to get your feedback.

I was also thinking over the weekend that a lot of this code could be skipped in many cases if we used getNextTransition/getPreviousTransition.

Some time zones may not implement these functions-- they're optional AFAIK. Seems like a better approach might be to use PlainDateTime math and then adjust the endpoint as needed. But let's discuss when we chat 1:1 next week.

[justingrant]

I did a deep-dive review of this logic today it seems solid. I thought I was seeing unexpected behavior in a few cases, but after digging into the polyfill a little more now I think the current behavior is fine.

One thing we should probably document at some point is the expected behavior when roundingIncrement > 1 and roundingMode: 'nearest'. Your current implementation, after I thought about it a while, is doing the right thing: if there's an odd number of smallestUnits between the start point and end point, then the tie-breaking only considers the smallestUnit in the middle. That's why the code below that returns PT0S is IMHO correct even though 2020-11-02T11:59[America/Los_Angeles] is farther from the start than the end (because there's an extra hour on Nov 1). With the current logic, if I'm understanding it correctly,, the tie-breaking calculation only takes the middle day (Nov 2) into account and that day has only 24 hours so therefore 11:59AM gets rounded down. Does this match how you're thinking about the problem?

Temporal.ZonedDateTime.from('2020-11-01[America/Los_Angeles]').until(
  '2020-11-02T11:59[America/Los_Angeles]', 
  { roundingIncrement: 3, smallestUnit: 'days', roundingMode: 'nearest' }
);
// => PT0S

The only remaining issue I see with rounding is #1197, because all the rounding code is using multiple rounds of math operations on non-integer Number values. Fixing this looks like will require a fairly significant refactor of RoundDuration to do all math on integers and only do one division at the end. For fixed-length units this seems straightfoward (just convert everything to nanoseconds) but I'm not sure how it'd work with variable-length units like days or months.

Am I correct that fixing #1197 doesn't require a spec change because the rounding abstract operations in the spec deal with "mathematical values" only and not Number until right before the user gets the result? So #1197 can be fixed later before Stage 3 but doesn't need to be fixed this week?