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

difference should always return a "top-heavy balanced" duration with largest-first order of operations #993

Closed
justingrant opened this issue Oct 15, 2020 · 17 comments · Fixed by #1123
Assignees
Labels
bug documentation Additions to documentation non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! spec-text Specification text involved

Comments

@justingrant
Copy link
Collaborator

justingrant commented Oct 15, 2020

difference is always supposed to return a "top-heavy balanced" duration, meaning it should be balanced except possibly for largestUnit. But that's not happening in the example below.

Temporal.Date.from('2020-03-30').difference(Temporal.Date.from('2020-01-31'), { largestUnit: 'months' });
// => P59D, but expected: P1M???D (??? = some number of days)
Temporal.Date.from('2020-03-31').difference(Temporal.Date.from('2020-01-31'), { largestUnit: 'months' });
// => P2M
@justingrant
Copy link
Collaborator Author

I put this in the "design decisions" milestone because it may be blocking resolution of #913.

@justingrant
Copy link
Collaborator Author

@pipobscure, @gibson042 - As this may block resolving #913, I'm pinging you here. What would you expect the value of ??? to be in the code example above?

@gibson042
Copy link
Collaborator

gibson042 commented Oct 15, 2020

Temporal.Date.from('2020-03-30').difference(Temporal.Date.from('2020-01-31'), { largestUnit: 'months' });
// => P59D, but expected: P1M???D (??? = some number of days)
Temporal.Date.from('2020-03-31').difference(Temporal.Date.from('2020-01-31'), { largestUnit: 'months' });
// => P2M

I honestly can't come up with a good value for the ???. 30 doesn't seem right because it only works with intermediate internal normalization, but 28 isn't great because it only works with different intermediate internal normalization. P59D might just be the best result.

Looking around, Moment returns P1M28D (actually P1M27DT23H because of DST) but does so from a naïve perspective that doesn't consider variable month length (cf. moment/moment#571 ), Luxon returns P1M30D, and date-fns returns P1M29DT1H (¯\_(ツ)_/¯). What happens in other languages?

@justingrant
Copy link
Collaborator Author

What happens in other languages?

Java returns P1M30D, or the equivalent negative duration if args are reversed. The algorithm Java uses seems to be something like this:

  1. Start with the smaller value: 2020-01-31
  2. Find the number of whole months that can be added to it without going past 2020-03-30. That's one month.
  3. Add one month to 2020-01-31 and constrain that result to a valid date, e.g. 2020-02-29
  4. Now find the number of whole days that can be added without going past the end. That's 30 days.
  5. Return P1M30D
LocalDate d1 = LocalDate.of(2020, 1, 31);
LocalDate d2 = LocalDate.of(2020, 3, 30); 
Period period = Period.between(d1, d2);
System.out.println("diff: " + period.toString());
// => P1M30D

.NET's TimeSpan type (analog to Temporal.Duration) only goes up to days so and doesn't include months, so it doesn't have this problem.

@justingrant
Copy link
Collaborator Author

justingrant commented Oct 15, 2020

Here's another interesting result from Java: it's not reversible in all cases. And I'm unsure my guess at the algorithm above is correct.

// NOT REVERSIBLE
LocalDate d1 = LocalDate.of(2019, 1, 29);
LocalDate d2 = LocalDate.of(2019, 3, 1); 
Period period = Period.between(d1, d2);
System.out.println("diff: " + period.toString());
// => P1M1D
Period period2 = Period.between(d2, d1);
System.out.println("diff: " + period2.toString());
// => P-1M-3D

// NOT REVERSIBLE
LocalDate d1 = LocalDate.of(2019, 1, 30);
LocalDate d2 = LocalDate.of(2019, 3, 29); 
Period period = Period.between(d1, d2);
System.out.println("diff: " + period.toString());
// => P1M29D
Period period2 = Period.between(d2, d1);
System.out.println("diff: " + period2.toString());
// => P-1M-30D

// REVERSIBLE
LocalDate d1 = LocalDate.of(2019, 1, 31);
LocalDate d2 = LocalDate.of(2019, 3, 30); 
Period period = Period.between(d1, d2);
System.out.println("diff: " + period.toString());
// => P1M30D
Period period2 = Period.between(d2, d1);
System.out.println("diff: " + period2.toString());
// => P-1M-30D

Here's what the documentation of Period.between says:

The period is calculated by removing complete months, then calculating the remaining number of days, adjusting to ensure that both have the same sign. The number of months is then split into years and months based on a 12 month year. A month is considered if the end day-of-month is greater than or equal to the start day-of-month. For example, from 2010-01-15 to 2011-03-18 is one year, two months and three days.

The result of this method can be a negative period if the end is before the start. The negative sign will be the same in each of year, month and day.

Translating this algorithm into Temporal pseudocode, I suspect that it provides the following guarantee:

let diff = date2.difference(date1, {largestUnit: 'months'});
date1.add(diff).equals(date2);  // always true

But it probably DOESN'T always provide these guarantees:

// subtracting from endpoint may not yield the same result as adding to the starting point
date2.subtract(diff).equals(date1);  // not always true

// reversing the arguments may not return the same result
let negative = date1.difference(date2, {largestUnit: 'months'});
diff.negated().equals(negative); // not always true

It seems like there are at least two levers we can play with in a difference algorithm:

  1. Whether order-of-operations is reversed when subtracting positive durations or adding negative durations
  2. Whether difference always starts from the smaller value and adds to get to the larger one and then tacks on a sign at the end, or whether it always starts from other and adds or subtracts depending on whether this is larger or smaller.

It'd be interesting to see if either of these levers would enable the additional reversibility assertions above. That said, I'm not exactly sure how reversing the OOO would work for difference. If I start with 2019-03-01 and want to get to 2019-01-30, then if I start by subtracting days then how do I know how many days to subtract in cases where the penultimate month has fewer days than the starting date? And, for a DateTime variation of this, how would away-from-zero rounding work when subtracting, and what reversibility guarantees would be expected when rounding is involved?

And honestly I'm not sure how critical these reversibility guarantees are. Seems like being clear about what's expected to work and what's not might be enough.

P59D might just be the best result.

I don't agree. If the user's intent was to get a zero-months duration, they would have used the default largestUnit which is days. The fact that the user opted into largestUnit: 'months' seems like a really clear intent that the result should have nonzero months. I think it's better to give the user what they asked for (nonzero months) and document the limitations of the result (e.g. not reversible if months are involved?) instead of returning the user something they explicitly didn't ask for: an unbalanced days-only duration with no months.

30 doesn't seem right because it only works with intermediate internal normalization, but 28 isn't great because it only works with different intermediate internal normalization.

What's the calculation you have in mind that gets to 30 or 28?

@gibson042
Copy link
Collaborator

P59D might just be the best result.

I don't agree. If the user's intent was to get a zero-months duration, they would have used the default largestUnit which is days. The fact that the user opted into largestUnit: 'months' seems like a really clear intent that the result should have nonzero months. I think it's better to give the user what they asked for (nonzero months) and document the limitations of the result (e.g. not reversible if months are involved?) instead of returning the user something they explicitly didn't ask for: an unbalanced days-only duration with no months.

But the result cannot have nonzero months without data loss. largestUnit doesn't mean "definitely nonzero", it means "definitely no larger unit and no overflowing smaller unit", which is the case here (albeit surprisingly so).

30 doesn't seem right because it only works with intermediate internal normalization, but 28 isn't great because it only works with different intermediate internal normalization.

What's the calculation you have in mind that gets to 30 or 28?

P1M30D would come from largest unit to smallest unit calculations with intermediate constraining (the calculation you described above): 2020-01-31 to 2020-03-30 is ≥P1M <P2M, [2020-01-31 + P1M = 2020-02-31 →] 2020-02-29 to 2020-03-30 is P30D, the result is therefore P1M30D.

P1M28D would come from largest unit to smallest unit calculations with intermediate balancing: 2020-01-31 to 2020-03-30 is ≥P1M <P2M, [2020-01-31 + P1M = 2020-02-31 →] 2020-03-02 to 2020-03-30 is P28D, the result is therefore P1M28D.

@pipobscure pipobscure self-assigned this Oct 15, 2020
@justingrant
Copy link
Collaborator Author

justingrant commented Oct 16, 2020

Meeting 2020-10-15:

  1. Order of operations throughout Temporal will be largest units first, even for subtraction of positive durations or adding negative durations.
    • 1.1 This is OK because default units for all types except YearMonth are always 'days' (or smaller) and with the default unit, all math will be reversible.
    • 1.2 Addition and subtraction operations must return the same as if the operation was decomposed into one operation for each unit. This means that for any inputs, the following should always be true:
    a.add(d, options).equals(a
      .add({years: d.years}, options)
      .add({months: d.months}, options)
      .add({weeks: d.weeks}, options)
      .add({days: d.days}, options)
      .add({hours: d.hours}, options)
      .add({minutes: d.minutes}, options)
      .add({seconds: d.seconds}, options)
      .add({milliseconds: d.milliseconds}, options)
      .add({microseconds: d.microseconds}, options)
      .add({nanoseconds: d.nanoseconds}, options)
    );
    • 1.3 One exception to above is ZDT add/subtract where time zone disambiguation will only happen after the entire date portion is calculated, i.e. after the .add({days: d.days}, options) step above. This is because RFC 5545 requires this behavior where DST disambiguation only happens after the date portion.
    • 1.4 The overflow option should be applied after every unit. So the following code should throw:
    Temporal.PlainDate.from('2020-01-31').add({months: 1, days: 1}, {overflow: 'reject'});
    // => throws
    Temporal.PlainDate.from('2020-01-31').add({months: 1, days: 1});
    // => 2020-03-01
    • 1.5 If the user opts into months or larger units (or is using ZonedDateTime math near a DST transition) then math will not always be reversible which is OK. Here's what to expect:
d = a.difference(b, {largestUnits: 'months'});
b.add(d).equals(a);  // will always be true, regardless of values of `a` and `b`
a.subtract(d).equals(b);  // may not be true for some values of `a` and `b`

// if there's rounding, then it's not reversible! 
dRounded = a.difference(b, {largestUnits: 'months', smallestUnit: 'days'});
b.add(dRounded).equals(a);  // may not be true, depending on `a` and `b`
  1. In difference, calculation will:

  2. Example of (2): Temporal.PlainDate.from('2019-03-29').difference('2019-01-30', {largestUnit: 'months')

    • 3.1 Start at 2019-01-31 and add one month to yield 2019-02-30 which constrains to 2019-02-28
    • 3.2 Now add 29 days to get 2019-03-29
    • 3.3 Final result is P1M29D
  3. Another example of (2): Temporal.PlainDate.from('2019-01-30').difference('2019-03-29', {largestUnit: 'months')

    • 3.1 Start at 2019-03-29 and subtract one month to yield 2019-02-29 which constrains to 2019-02-28
    • 3.2 Now subtract 30 days to get 2019-03-29
    • 3.3 Final result is -P1M30D. Note that this is different from calculating difference in the reverse order in (3) above.
  4. Here's a few test cases to validate the expected behavior above. Each of these test cases should take this form:

const d = thisValue.difference(otherValue, {largestUnits: 'months'});
equal (d.toString(), expected.toString());
equal (otherValue.add(d).toString(), thisValue.toString()); 
thisValue otherValue expected
2019-03-01 2019-01-29 P1M1D
2019-01-29 2019-03-01 -P1M3D
2019-03-29 2019-01-30 P1M29D
2019-01-30 2019-03-29 -P1M29D
2019-03-30 2019-01-31 P1M30D
2019-01-31 2019-03-30 -P1M28D
2019-03-31 2019-01-31 P2M
2019-01-31 2019-03-31 -P2M

@ptomato ptomato added documentation Additions to documentation non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! spec-text Specification text involved labels Oct 16, 2020
@gibson042
Copy link
Collaborator

Are you sure about all the test cases?

  • 2019-03-29 to 2019-01-30 seems to me like -P1M29D.
  • 2019-03-30 to 2019-01-31 seems to me like -P1M29D.

@justingrant
Copy link
Collaborator Author

justingrant commented Oct 18, 2020

  • 2019-03-29 to 2019-01-30 seems to me like -P1M29D.

You're right. I edited this test case above.

  • 2019-03-30 to 2019-01-31 seems to me like -P1M29D.

My original test case was wrong, but I believe that this result should be -P1M28D, not -P1M29D:

  • Start from 2019-03-30
  • Subtract one month to get 2019-02-30, constrained to 2019-02-28
  • Subtract 28 days to get to 2019-01-31

I edited accordingly.

BTW, I got both of those cases above wrong because I was assuming that Java's algorithm matches what we proposed. However, this is not the case. Java is using a different calculation methods depending on whether the difference is being calculated from larger to smaller vs. smaller to larger. I think our proposed method is better because it's more predictable.

@justingrant justingrant changed the title months missing from results of Date.difference with largestUnit: months difference should always return a "top-heavy balanced" duration with largest-first order of operations Oct 19, 2020
@justingrant
Copy link
Collaborator Author

I added some additional clarifying content to the proposal above to incorporate what I learned from after implementing this change in ZoneDateTime:

  • 1.2 Addition and subtraction operations must return the same as if the operation was decomposed into one operation for each unit. This means that for any inputs, the following should always be true:
a.add(d, options).equals(a
  .add({years: d.years}, options)
  .add({months: d.months}, options)
  .add({weeks: d.weeks}, options)
  .add({days: d.days}, options)
  .add({hours: d.hours}, options)
  .add({minutes: d.minutes}, options)
  .add({seconds: d.seconds}, options)
  .add({milliseconds: d.milliseconds}, options)
  .add({microseconds: d.microseconds}, options)
  .add({nanoseconds: d.nanoseconds}, options)
);
  • 1.3 One exception to above is ZDT add/subtract where time zone disambiguation will only happen after the entire date portion is calculated, i.e. after the .add({days: d.days}, options) step above. This is because RFC 5545 requires this behavior where DST disambiguation only happens after the date portion.
  • 1.4 The overflow option should be applied after every unit. So the following code should throw:
Temporal.PlainDate.from('2020-01-31').add({months: 1, days: 1}, {overflow: 'reject'});
// => throws
Temporal.PlainDate.from('2020-01-31').add({months: 1, days: 1});
// => 2020-03-01

justingrant added a commit to justingrant/proposal-temporal that referenced this issue Oct 20, 2020
This commit implements the largest-units-first behavior defined in tc39#993.
This includes both addition and subtraction.

Note that until tc39#993 is implemented, this commit inefficiently calls
`DateTime.prototype.add` (or `subtract`) multiple times. This can be
removed once tc39#993 lands.
justingrant added a commit to justingrant/proposal-temporal that referenced this issue Oct 21, 2020
This commit implements the largest-units-first behavior defined in tc39#993.
This includes both addition and subtraction.

Note that until tc39#993 is implemented, this commit inefficiently calls
`DateTime.prototype.add` (or `subtract`) multiple times. This can be
removed once tc39#993 lands.
@ptomato
Copy link
Collaborator

ptomato commented Oct 28, 2020

If it's the case that this change makes subtract(duration, options) always identical to add(duration.negated(), options), then I think it should include removing Temporal.Calendar.dateSubtract() and using Temporal.Calendar.dateAdd() in the implementations of subtract().

@ptomato
Copy link
Collaborator

ptomato commented Nov 2, 2020

@pipobscure Have you been able to make any progress on this?

@pipobscure
Copy link
Collaborator

Not enough and I won’t get back to it before the weekend. So if you think you’ll be quicker, please!!!

ptomato added a commit that referenced this issue Nov 3, 2020
Two tests are skipped due to the change in order-of-operations that will
land in #993.

Co-authored-by: Justin Grant <justingrant@users.noreply.github.com>

See: #569
Ms2ger pushed a commit that referenced this issue Nov 3, 2020
Two tests are skipped due to the change in order-of-operations that will
land in #993.

Co-authored-by: Justin Grant <justingrant@users.noreply.github.com>

See: #569
@justingrant
Copy link
Collaborator Author

justingrant commented Nov 4, 2020

I'm almost done with a PR. One question: in since(), should relativeTo be this or other? This matters because rounding and balancing may work differently for months and years depending on whether we start from other or start from this.

A good way to understand the choice is to choose which of the comparisons below should always be true. Only one can always be true for months and years largestUnit. (For the default options they're both true.)

d = a.since(b, {largestUnit: 'years'));
a.subtract(d).equals(b) === true; // if `relativeTo` is `this`
b.add(d).equals(a) === true; // if `relativeTo` is `other`

A) Arguments in favor of relativeTo is this:

  • Simpler to explain & understand how rounding works ("All rounding is relative to this")
  • The data flow (from this to other) is the same between until or since, which IMHO makes it easier to reason about
  • Developers already have to learn that a.add(d, {largestUnit: years}).equals(b) could be true but b.subtract(d, {largestUnit: years}).equals(a) could be false due to end-of-month constraining , lunar/lunisolar caladars, and DST transitions. This directional difference is the same concept applied to since/until instead of add/subtract, so it may be familiar.

B) Arguments in favor of relativeTo is other:

  • a.until(b) and b.since(a) always return the same duration, which seems intuitive.
  • Ensures that refactoring a.until(b) to b.since(a) won't cause bugs. This seems like it should be a safe refactor, but if (A) is implemented this "refactor" would actually emit different results for some dates. Even though this is a similar concept as a.add(d) vs b.subtract(d), the add/subtract case doesn't have a corresponding refactoring footgun.
  • Personally, I find it easier to think of addition instead of subtraction. Explaining a.since(b) returns "the duration you need to add to b to get a" feels easier to understand than "the duration you need to subtract from a to get b".
  • If we need a tie-breaker: I believe that this option will be easier to implement

My weakly-held opinion is that arguments in favor of (B) are somewhat more compelling, because the refactoring issue seems significant and otherwise they both seem fairly evenly matched.

Therefore, unless anyone objects, I'll plan to implement relativeTo is other tomorrow.

if it matters, I believe that the current polyfill is a mixed implementation: rounding in since is relative to this but arithmetic starts with other. I feel strongly that the same direction should be used for rounding and subtraction, because mixing directions makes it really hard to understand what since is doing when months are involved.

@pipobscure @gibson042 @ptomato - FYI

EDIT - I updated this post to clarify the proposed options.

@gibson042
Copy link
Collaborator

I think it would be unexpected and weird if one method started from its receiver but the other method started from it argument. Having since and until differ in that behavior seems like a recipe for practicioner confusion. I'd much rather see Temporal structure things such that the correct method to use is determined by which value is intended as the starting point.

As for whether the starting point should be the receiver for both methods or the argument for both methods, I don't have any intuition and my only opinion is a preference for whichever results in the most straightforward code for use cases we've identified (e.g., in the cookbook).

@ptomato ptomato assigned justingrant and unassigned pipobscure Nov 4, 2020
@ptomato
Copy link
Collaborator

ptomato commented Nov 4, 2020

We already decided on "A" for rounding;

  • earlier.until(later) rounds the difference relative to earlier
  • later.until(earlier) rounds the negative of the difference relative to later

Given that decision, in my opinion there is only one self-consistent option, which is to pick "A" consistently, so that earlier.until(later) gives you the difference that you need to add to earlier to get later, and later.until(earlier) gives you the difference that you need to subtract from later to get earlier.

@justingrant
Copy link
Collaborator Author

OK I implemented (A) in #1123. That PR is ready for review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug documentation Additions to documentation non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! spec-text Specification text involved
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants