Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Duration::total incorrect result with DST #2817

Closed
arshaw opened this issue Apr 13, 2024 · 2 comments
Closed

Duration::total incorrect result with DST #2817

arshaw opened this issue Apr 13, 2024 · 2 comments
Assignees

Comments

@arshaw
Copy link
Contributor

arshaw commented Apr 13, 2024

Duration::total is mishandling DST gaps with unit is 'day'

I realized this when running this test failing for my polyfill. Portable recreation:

var hours25 = new Temporal.Duration(0, 0, 0, 0, 25);
var relativeTo = Temporal.ZonedDateTime.from("2024-03-09T02:30[America/New_York]")
var totalDays = hours25.total({ unit: "days", relativeTo })
// ACTUAL: 1.0416666666666667 (1 + 1 / 24)
// EXPECTED: 1.043478260869565216 (1 + 1 / 23)

// why EXPECTED?
{
  var start = relativeTo.add({ days: 1 }) // 2024-03-10T03:30:00-04:00[America/New_York]
  var middle = relativeTo.add(hours25) // 2024-03-10T04:30:00-04:00[America/New_York]
  var end = relativeTo.add({ days: 2 }) // 2024-03-11T02:30:00-04:00[America/New_York]
  console.log(
    'EXPECTED:',
    1 +
    Number(middle.epochNanoseconds - start.epochNanoseconds) /
      Number(end.epochNanoseconds - start.epochNanoseconds)
  ) // 1.043478260869565216
  console.log(
    'hours in day:',
    Number(end.epochNanoseconds - start.epochNanoseconds) /
      3600000000000
  ) // 23
}

// why ACTUAL?
{
  var start = relativeTo.add({ days: 1 }) // 2024-03-10T03:30:00-04:00[America/New_York]
  var middle = relativeTo.add(hours25) // 2024-03-10T04:30:00-04:00[America/New_York]
  var end = start.add({ days: 1 }) // 2024-03-11T02:30:00-04:00[America/New_York]
  //             ^incorrect. should not be added to start. instead, base off relativeTo
  console.log(
    'ACTUAL:',
    1 +
    Number(middle.epochNanoseconds - start.epochNanoseconds) /
      Number(end.epochNanoseconds - start.epochNanoseconds)
  ) // 1.0416666666666667
}

The problem is happening here:

let oneDayFarther = AddDaysToZonedDateTime(
relativeResult.instant,
relativeResult.dateTime,
timeZoneRec,
calendar,
sign
);
let dayLengthNs = TimeDuration.fromEpochNsDiff(oneDayFarther.epochNs, relativeResult.epochNs);

I'd recommend reworking the Duration::total() system. This should happen while handling the reworks suggested in #2792.

@ptomato
Copy link
Collaborator

ptomato commented May 1, 2024

OK, I buy this explanation. The correct answer seems like 1+1/23 and that is also what results from the algorithm we are discussing in #2792. I still need to look at a few other cases though.

@ptomato ptomato self-assigned this May 1, 2024
ptomato added a commit that referenced this issue May 7, 2024
These optimizations were developed by Adam Shaw:
https://gist.github.com/arshaw/36d3152c21482bcb78ea2c69591b20e0

It does the same thing as previously, although fixes some incidental edge
cases that Adam discovered. However, the algorithm is simpler to explain
and to understand, and also makes fewer calls into user code.

It uses three variations on a bounding technique for rounding: computing
the upper and lower result, and checking which one is closer to the
original duration, and 'nudging' the duration up or down accordingly.

There is one variation for calendar units, one variation for rounding
relative to a ZonedDateTime where smallestUnit is a time unit and
largestUnit is a calendar unit, and one variation for time units.

RoundDuration becomes a lot more simplified, any part of it that was
complex is now split out into the new RoundRelativeDuration and
BubbleRelativeDuration operations, and the three 'nudging' operations.

The operations NormalizedTimeDurationToDays, BalanceTimeDurationRelative,
BalanceDateDurationRelative, MoveRelativeDate, MoveRelativeZonedDateTime,
and AdjustRoundedDurationDays are no longer needed. Their functionality is
subsumed by the new operations.

Closes: #2792
Closes: #2817
@ptomato
Copy link
Collaborator

ptomato commented May 7, 2024

Addressed as part of @arshaw's review comments on #2758.

ptomato added a commit that referenced this issue May 10, 2024
These optimizations were developed by Adam Shaw:
https://gist.github.com/arshaw/36d3152c21482bcb78ea2c69591b20e0

It does the same thing as previously, although fixes some incidental edge
cases that Adam discovered. However, the algorithm is simpler to explain
and to understand, and also makes fewer calls into user code.

It uses three variations on a bounding technique for rounding: computing
the upper and lower result, and checking which one is closer to the
original duration, and 'nudging' the duration up or down accordingly.

There is one variation for calendar units, one variation for rounding
relative to a ZonedDateTime where smallestUnit is a time unit and
largestUnit is a calendar unit, and one variation for time units.

RoundDuration becomes a lot more simplified, any part of it that was
complex is now split out into the new RoundRelativeDuration and
BubbleRelativeDuration operations, and the three 'nudging' operations.

The operations NormalizedTimeDurationToDays, BalanceTimeDurationRelative,
BalanceDateDurationRelative, MoveRelativeDate, MoveRelativeZonedDateTime,
and AdjustRoundedDurationDays are no longer needed. Their functionality is
subsumed by the new operations.

Closes: #2792
Closes: #2817
ptomato added a commit that referenced this issue May 13, 2024
These optimizations were developed by Adam Shaw:
https://gist.github.com/arshaw/36d3152c21482bcb78ea2c69591b20e0

It does the same thing as previously, although fixes some incidental edge
cases that Adam discovered. However, the algorithm is simpler to explain
and to understand, and also makes fewer calls into user code.

It uses three variations on a bounding technique for rounding: computing
the upper and lower result, and checking which one is closer to the
original duration, and 'nudging' the duration up or down accordingly.

There is one variation for calendar units, one variation for rounding
relative to a ZonedDateTime where smallestUnit is a time unit and
largestUnit is a calendar unit, and one variation for time units.

RoundDuration becomes a lot more simplified, any part of it that was
complex is now split out into the new RoundRelativeDuration and
BubbleRelativeDuration operations, and the three 'nudging' operations.

The operations NormalizedTimeDurationToDays, BalanceTimeDurationRelative,
BalanceDateDurationRelative, MoveRelativeDate, MoveRelativeZonedDateTime,
and AdjustRoundedDurationDays are no longer needed. Their functionality is
subsumed by the new operations.

Closes: #2792
Closes: #2817
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants