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

Proposal: Rounding method (and rounding for difference and toString) for non-Duration types #827

Closed
11 tasks done
justingrant opened this issue Aug 15, 2020 · 45 comments
Closed
11 tasks done
Assignees

Comments

@justingrant
Copy link
Collaborator

justingrant commented Aug 15, 2020

This proposal is half of the rounding problem focused on non-Duration types. The other half (rounding methods on the Duration type itself) is in #856. Originally both halves were contained in #789 but we split into two proposals because the Duration parts took longer to reach consensus.

Summary

This proposal:

  • Adds a round() method to every non-Duration Temporal type that supports math operations.
  • Adds rounding options to the difference method of the same types, in order to support rounding of the results of difference.
  • Defines rounding options that will be used by the Duration type too (see Proposal: rounding and balancing for Duration type (replaces #789) #856 that depends on this proposal).

Open Issues to Resolve

  • Should we support rounding of months and years units? (see discussion in comments) Note that weeks and era are already not supported (see -- only months/years are in question. NO
  • What should be the default rounding mode? Half up? Half even? HALF UP
  • Should with() have rounding options? (see discussion in comments) NO
  • Should roundingIncrement require an exact divisor of the next-largest unit? YES, EXCEPT: ABSOLUTE TYPES HAVE SPECIAL RULES, and LOCALDATETIME FOR HOURS USES CLOCK HOURS, NOT ABSOLUTE HOURS.
  • Related: how should roundingIncrement handle cases where the next-largest unit has a variable size? (e.g. month length in days, year length in months for non-ISO calendars) N/A
  • Is it OK that toString includes rounding options? MAYBE - MOVED to Why do toString methods truncate? #329
  • Is it OK that toString controls decimal precision of sub-second units (and whether to show seconds at all) based on smallestUnit? MAYBE - MOVED
  • Should toLocaleString accept the same rounding options as toString()? N/A
  • Is difference ready? Or does it need to wait for the rounding in Duration proposal (Proposal: rounding method & total method for Duration type #789) to be fully resolved? FWIW, I'd prefer not to block it if we don't have to. YES
  • Because the default (see below) for smallestUnit is the smallest unit that the type supports, calling round() with no parameters will return a clone of the same instance but won't change the value. Is this OK? NO.. IT'S REQUIRED.
  • difference could theoretically choose either this or other as the reference point for rounding. I'm assuming that it should use this. YES. HAS CONSENSUS.

Sample Use Cases

round(options)

  • Round the current time to the nearest 5 minutes
Temporal.now.time().round({ smallestUnit: 'minute', roundingIncrement: 5 })
  • What's the closest Saturday to this Date? - We removed week-related rounding from the scope of this proposal. The workaround is to calculate using dayOfWeek directly.
  • Remove the seconds and smaller units from this Time
time.round({ smallestUnit: 'minute', roundingMode: 'trunc' })
  • Round this time to the nearest second
time.round({ smallestUnit: 'second' })
  • What was the first day of this calendar quarter? - We chose to remove rounding for weeks and larger units due to their complexity (especially in non-ISO calendars) and their relatively easy workarounds. The workaround would be to do the 3-month rounding math directly using the property values and with and/or from.
  • How many billable hours did a lawyer spend on a phone call, rounded up to the nearest 5 minutes?
endAbsolute.difference(startAbsolute, { smallestUnit: 'minutes', roundingIncrement: 5, roundingMode: 'ceil' })
  • How many days was a rental car rented for, rounded up to the nearest hour?
endLocalDateTime.difference(startLocalDateTime, { smallestUnit: 'hour', roundingMode: 'ceil' })

Details

1. round() for non-Duration types

  • 1.1 Rounding is focused on time. The following types that include time will get a round method: DateTime, Absolute, Time, and LocalDateTime.
    • 1.1.1 Because we'll only be rounding times, not dates, the Date, YearMonth, and MonthDay types won't get a round() method.
  • 1.2 Duration will also likely get a round method, but that's out of scope for this proposal. See Proposal: rounding method & total method for Duration type #789.
  • 1.3 round() will accept only one parameter, options, which is a property bag of options that control rounding. Each option is described below.
    • 1.3.1 options is a required parameter because one of the options, smallestUnit (see below) cannot be omitted.
  • 1.4 We should use the same rounding modes and option names that will be used by Decimal and by Intl.NumberFormat (What rounding modes to include? proposal-intl-numberformat-v3#7). @sffc is driving Intl.NumberFormat so alignment should be do-able. Per @sffc, we don't need to wait for either proposal-- the first of the three out of the gate will define naming used by the others.

2. smallestUnit option

  • 2.1 The smallestUnit property defines where rounding happens. All smaller time units will be zero in the result.
  • 2.2 There is no default. A RangeError will be thrown if smallestUnit is omitted.
  • 2.3 If smallestUnit is larger than largestUnit, then a RangeError should be thrown.
  • 2.4 smallestUnit should accept both plural and non-plural unit names because non-Duration types' fields aren't pluralized. This may be a reason to accept either plural or non-plural names in other places too. See Naming of Temporal.Duration fields #325.
  • 2.5 Even though both plural and non-plural units will be accepted, the docs should show singular units in samples because non-Duration field names are singular.
  • 2.7 era or any non-ISO calendar custom fields will throw a RangeError, because it's not possible to round those units without significant changes to Temporal.Calendar. Also that there's likely minimal demand for rounding these units.
  • 2.8 The Absolute type doesn't have a time zone so we'll limit its smallestUnit to 'minutes' or smaller to support the primary Absolute rounding use case which is to handle underlying storage that has limited precision at the low end.
  • 2.9 For Time, smallestUnit can be any valid unit for this type: 'hours' or smaller.
  • 2.10 For DateTime and LocalDateTime, smallestUnit must be 'days' or smaller. We decided to exclude larger units because the rounding behavior of weeks and months is confusingly calendar-dependent.

5. roundingMode option

  • 5.1 This option defines how the remainder is handled: e.g. round up, round down, round to nearest, etc. There are a surprisingly large number of possible rounding modes (e.g. round down vs. round towards zero for negative numbers, how to handle 0.5 remainders, etc.).
    • 5.1.1 There are two other potential users of rounding modes in JS: the Decimal proposal and Intl.NumberFormat V3. Temporal will align with these proposals for naming and modes available, so that all of JS would have consistent rounding syntax.
    • 5.1.2 Per @sffc, NumberFormat V3 is the furthest ahead of all three proposals so we'll tentatively plan for the naming and rounding-mode decisions to be made there first, although if Temporal makes them first them @sffc will help to align Intl.
  • 5.2 Possible option names include (pending alignment with those other proposals): "roundingMode", "rounding", "roundingType", "roundingMethod". This proposal uses "roundingMode" but the name may change.
  • 5.3 The following rounding modes will be supported. Initial naming is aligned with the Math object, although per above the names may change as we align with Intl.
    • 5.3.1 'ceil' - round up towards +∞
    • 5.3.2 'floor' - round down towards -∞
    • 5.4.3 'trunc' - round towards zero. Note that "zero" implies "the big bang", not UNIX epoch or 1 B.C. This means that 'floor' and 'trunc' act the same for non-Duration types because zero and -∞ are the same from Temporal's point of view. Note that they will not act differently when used in the Duration type (see Proposal: rounding and balancing for Duration type (replaces #789) #856) because durations can be negative.
    • 5.4.4 'nearest' - round to nearest integer, with 0.5 rounded away from zero. This is named differently from round, to differentiate from Math.round that rounds 0.5 remainders towards +∞ which is often unexpected for negative numbers per MDN:

If the fractional portion is exactly 0.5, the argument is rounded to the next integer in the direction of +∞. Note that this differs from many languages' round() functions, which often round this case to the next integer away from zero, instead giving a different result in the case of negative numbers with a fractional part of exactly 0.5.

  • 5.4 The default rounding mode is 'nearest' which means "half away from zero" because it's least surprising for a method called round. Note that the name of this option may change depending on what is decided in What rounding modes to include? proposal-intl-numberformat-v3#7 (comment). @sffc owns coordinating with Intl and finalizing both rounding mode naming and which rounding modes are offered in both places.
  • 5.6 It's possible that Intl may choose to add additional rounding modes, and IMHO we should adopt those if they're added. However, it's a safe assumption that the 4 modes above will be included.

6. roundingIncrement option

  • 6.1 The roundingIncrement allows rounding with a coarser granularity than one unit, e.g. "nearest 15 minutes", "nearest second".
  • 6.2 The default rounding increment is 1
  • 6.3 This option will be limited to exact divisors of the next-largest unit. For example, if smallestUnit is minute, then roundingIncrement could be either 1 (default), 2, 3, 4, 5, 6, 10, 12, 15, 20, or 30. Others must throw a RangeError.
  • 6.4 There's been discussion of wanting to enable future non-ISO calendars that can introduce custom time measurement, which might add the same problem about variable number of minutes/seconds/etc. @sffc believes that this could be resolved, either by using increments relative to the calendar system, or we could reset the increment at each boundary of the next-largest unit. So we won't worry about non-ISO calendars relative to this feature.
  • 6.5 Regardless of rounding mode or increment, the rounding increment will be measured in ascending order starting from zero of smallestUnit.
  • 6.6 When applied to LocalDateTime, roundingIncrement will use clock time, not absolute time, to determine the result. If a DST transition is inside the increment, the absolute length of the increment can be longer or shorter (by the length of the DST transition) than the user-specified roundingIncrement value. Also, if the result's clock time is ambiguous, then it's disambiguated using the default ('compatible') disambiguation option. For example:
ldt = Temporal.LocalDateTime.from('2020-03-08T03:30-07:00[America/Los_Angeles]');
ldt.round({ smallestUnit: 'hour', roundingIncrement: 3, roundingMode: 'ceil' }); 
  // => 2020-03-08T06:00-07:00[America/Los_Angeles] 
  //      (result is 06:00 even though it's only 5 hours past midnight)
ldt.round({ smallestUnit: 'hour', roundingIncrement: 2, roundingMode: 'trunc' }); 
  // => 2020-03-08T03:00-07:00[America/Los_Angeles]
  //      (result is 03:00 because 02:00-08:00 is disambiguated to 03:00-07:00)
  • 6.7 Absolute has a few differences from behavior of other types:
    • 6.7.1 As noted above, rounding on Absolute is limited to minutes or smaller units. If developers want to round an Absolute to an hour, they can just use {roundingIncrement: 60}.
    • 6.7.2 The reference point for rounding increments for Absolute is the UNIX epoch.
    • 6.7.3 All Absolute rounding increments must divide evenly into one 24-hour day (86400 seconds). Unlike the increments in the other types' round() methods, Absolute rounding increments can also be equal (not less than like other types) to 86400 seconds.

7. Rounding options in non-Duration difference methods

  • 7.1 Currently, difference methods throughout Temporal accept a largestUnit option.
  • 7.2 With this proposal, types that get a round method will also enhance the difference method to also accept the same options that round accepts: smallestUnit, roundingMode, and roundingIncrement.
  • 7.3 These options will behave the same as in round, with the only exceptions noted below.
    • 7.3.1 smallestUnit defaults to nanoseconds and is optional.
  • 7.4 If largestUnit is not specified but smallestUnit is larger than its default value, then largestUnit should be adjusted to be the same as smallestUnit.
    • 7.4.1 However, if smallestUnit is larger than an explicitly specified largestUnit, then a RangeError should be thrown.
  • 7.5 Unit names are plural e.g. 'days' in the docs, but both plural and singular units should be accepted.
  • 7.6 All units supported by the existing largestUnit option will be supported in smallestUnit too.
  • 7.7 Weeks overlap with days and months in a duration. While there may be some cases where users might want weeks together with months and/or days (e.g. "3 months, 2 weeks, and 4 days") that's an uncommon case relative to the "3 months and 18 days" result that is likely expected. We'll handle these cases by setting weeks to zero when there's potential ambiguity between weeks vs. days/months. Specifically: if largestUnit is 'month' or 'year' and smallestUnit is 'day' or smaller, then weeks will always be zero and only days and/or months will be included in the result. Edge cases that are NOT ambiguous are noted below.
    • 7.7.1 If smallestUnit is 'week', then there is no ambiguity and any remainder will go into week.
    • 7.7.2 If largestUnit is 'day', then there is no ambiguity because weeks are excluded and week will be zero.
    • 7.7.3 In the degenerate case of { largestUnit: 'week', smallestUnit: 'day' } then there is also no ambiguity so both fields will be populated.
@sffc
Copy link
Collaborator

sffc commented Aug 15, 2020

1.1: Within the context of time scale units (Date, DateTime, Time, LocalDateTime, YearMonth, MonthDay), I don't find a round method to be compelling with smallestUnit larger than days. This is especially the case because rounding to date units will require going through the calendar system: how about rounding to the nearest Japanese Era? The nearest month where the month is non-Gregorian? I think .with() gets the job done reasonably well for date units, and it already handles calendar systems. Please note that my comment applies only to time scale units, not duration units, which are being discussed in #789.

Using .with() is painful for time units because there are so many time units that you need to manually balance and zero out, and because we've established numerous use cases where you want to round to an increment instead of just a whole unit.

2.6: Not relevant if we drop week/month/year rounding from time scale units, but on smallestUnit: 'week', ISO 8601-1 defines weekday 1 as Monday, and I think it's probably okay to follow that precedent.

6.2: A legitimate concern, but not relevant if we drop week/month/year rounding from time scale units.

6.3: If we do add custom time measurements, we can figure out how to handle this case. I don't think it will be that painful. I think the usefulness of rounding increments outweighs the possibility of custom time measurements.

6.4: I've convinced myself that people will want rounding increments, and that we should support them in V1.

7: I think rounding on the difference method is fundamentally different from a round method on Temporal.[Date]Time, because it is rounding a duration unit, not a time scale unit. I think we should put the difference discussion in #789.

8: I don't see this to be necessary, because you can just do dateTime.round({ smallestUnit: "hours" }).toString() instead of dateTime.toString({ smallestUnit: "hours" }). If we were to add the options bag, I would expect it to also go on toLocaleString(), which raises questions about how to resolve which options go to round and which ones go to the Intl.DateTimeFomat constructor.

@justingrant
Copy link
Collaborator Author

justingrant commented Aug 15, 2020

UPDATE: I updated the response below to point to specific sections of the proposal that were updated based on @sffc's feedback. Also added a few more details to my response below.

YearMonth, MonthDay

BTW, my intent was that round would only be available on types that already had math. So not YearMonth and MonthDay. Proposal is now updated (1.1.1) to clarify this.

rounding to date units will require going through the calendar system: how about rounding to the nearest Japanese Era?

I think that era and custom units in non-ISO calendars should be out of scope, both for limited usage and because they introduce problems like you mentioned. I guess we could always choose to extend Calendar later to add a round() method if there was sufficient demand, but custom fields is such an oddball case that it seems fine to omit.

Proposal is now updated (2.8) to clarify that era and custom units are not supported.

1.1: Within the context of time scale units (Date, DateTime, Time, LocalDateTime, YearMonth, MonthDay), I don't find a round method to be compelling with smallestUnit larger than days.

I don't feel strongly about this one way or the other. I've already assumed that 'weeks' is out of scope (and now also 'era' and custom fields) so adding 'months' and 'years' to the excluded list isn't a huge deal because the workarounds are easy.

If we don't support those units fo rounding, then we should probably document a workaround, for example:

  • Months
    • 'trunc' - Set day to 1 and time units to 0.
    • 'round' - Round down and add one month.
    • 'nearest' - Calculate midpoint in month using .daysInMonth (and hoursInDay if LocalDateTime, 24 otherwise)
  • Years
    • 'trunc' - Set day and month to 1 and time units to 0.
    • 'round' - Round down and add one year.
    • 'nearest' - Calculate midpoint in year using .daysInYear (and hoursInDay if LocalDateTime, 24 otherwise)

Do these algorithms work for all calendars? And if so, does that make you want to retain months and years as units that can be rounded?

BTW, with cross-calendar (if one is ISO) difference, we'd use the non-ISO calendar, following the lead of #822.

on smallestUnit: 'week', ISO 8601-1 defines weekday 1 as Monday, and I think it's probably okay to follow that precedent.

Because week-start-day varies so much across countries (even those that use the gregorian calendar), I think it'd be more confusing than valuable to do week rounding. IMHO, the potential bug rate seems too high relative to value. If we had the ability in Intl to fetch a locale's week-start date, and if we required callers of round to specify the week start index, then I'd be OK to have weeks here, but seems like a better time to add this support would be after Intl's week-start-index work is complete.

8: I don't see this to be necessary, because you can just do dateTime.round({ smallestUnit: "hours" }).toString()

AFAIK, the main use cases are to control precision on fractional seconds and to control whether seconds are emitted at all.
round().toString() won't handle those cases. #329 has some discussion on this. I think I've also seen these use cases mentioned elsewhere but can't find the issues.

If we were to add the options bag, I would expect it to also go on toLocaleString()...

Yeah, I was thinking about toLocaleString but forgot to add that as an open issue in the proposal. Thanks for catching this. My initial take is this:

  • toString is for emitting machine-readable formats for interop with other software.
  • toLocaleString is for emitting human-readable formats for display in a UI.

If that distinction makes sense, then controlling which units are shown in the output seems reasonable for toString with the caveat that what's emitted must also be a valid machine-readable format. This limitation doesn't apply for toLocaleString, where it's totally fine to only display the year, for example. Or to display additional units like "day of week" that don't belong in an ISO string.

...which raises questions about how to resolve which options go to round and which ones go to the Intl.DateTimeFomat constructor.

Could toLocaleString accept smallestUnit, roundingMode, and roundingIncrement? I admittedly don't know the toLocaleString options well enough to have an opinion yet.

I added a new section (9) in the proposal as a placeholder to track toLocaleString behavior.

6.3: If we do add custom time measurements, we can figure out how to handle this case. I don't think it will be that painful. I think the usefulness of rounding increments outweighs the possibility of custom time measurements.

6.4: I've convinced myself that people will want rounding increments, and that we should support them in V1.

OK! I updated (6) to include this feature instead of postponing it.

Out of curiosity, how do you think we could support variable-time calendars with increments? I'd like to include this explanation in the proposal.

7: I think rounding on the difference method is fundamentally different from a round method on Temporal.[Date]Time, because it is rounding a duration unit, not a time scale unit. I think we should put the difference discussion in #789.

AFAIK, the obstacle on the Duration side is relativeTo, not rounding. In difference, we always know the starting point so I'm not sure we need to wait for resolution on relativeTo issues with Duration. So I'm not sure there's remaining ambiguity or disagreement around difference behavior, but I totally could be missing something. Do you have a specific problem use case in mind?

I guess one thing that theoretically could be part of difference could be specifying whether this or other plays the role of relativeTo. But honestly this seems like a low priority; we can just stipulate that this is always used as relativeTo, which is analogous to how LocalDateTime will handle calculation order with difference-- we'll just choose to start the calculation from the this end and document which end. If the user wants to start on the other end, they can call other.difference(this) instead of this.difference(other). Reversing the order is just as much work for the caller as using an option, so (channeling @ptomato) I'm not sure we need an option to reverse the order for rounding or any other reason.

Are there any other rounding options that would be needed for difference?

@gilmoreorless
Copy link
Contributor

This all seems pretty sensible, but I have a couple of queries.

5.6 It's possible that Intl may choose to add additional rounding modes, and IMHO we should adopt those if they're added. However, it's a safe assumption that the 4 modes above will be included, with one possible exception: the round-to-nearest mode may use the "round up 0.5 for even numbers, and down for odd numbers" strategy.

As a layperson using the Temporal API, I would be highly confused by a default rounding method that rounds 0.5 values in different directions depending on the non-fractional value.

I have a fairly limited understanding of the "half-to-even" and "half-to-odd" rounding methods, based primarily on the Wikipedia article on rounding 😉. But from I can see, the methods are designed to reduce total error when summing multiple rounded values. I don't think that scenario really applies to these Temporal methods, and it's definitely not consistent with any current JS Math functions.

I don't have a problem with "half-to-even" rounding being added as an extra option for those who want it, but I feel strongly that it shouldn't be the default.

6.3 This option will be limited to exact divisors of the next-largest unit. For example, if smallestUnit is minute, then roundingIncrement could be either 1 (default), 2, 3, 4, 5, 6, 10, 12, 15, 20, or 30. Other must throw a RangeError.

How would this work with smallestUnit of hours when using LocalDateTime? Depending on the time zone and day, there could be 23, 23.5, 24, 24.5, or 25 hours in a day (or even more variance for one-off offset changes).

Likewise, how would it work with smallestUnit of days since the number of days in a month is highly variable?

@sffc
Copy link
Collaborator

sffc commented Aug 16, 2020

Out of curiosity, how do you think we could support variable-time calendars with increments? I'd like to include this explanation in the proposal.

My initial thought would be that we throw an exception if the increment is not a divisor. Alternatively, we could define behavior that an increment "resets" at the unit mark; for example, a 25-minute increment in a 60-minute hour means that the valid minute values after rounding are 0, 25, and 50. So, a 10-minute increment in a hypothetical 45-minute hour could produce minute values of 0, 10, 20, 30, and 40.

AFAIK, the obstacle on the Duration side is relativeTo, not rounding.

The other challenge is that unlike time scale units, durations need not be balanced. largestUnit is a thing, unlike time scale units. Since .difference rounding implies duration rounding (which must also include balancing), we should think about it in the context of duration rounding.

I don't have a problem with "half-to-even" rounding being added as an extra option for those who want it, but I feel strongly that it shouldn't be the default.

Half-up, not half-even, is already the default on Intl, and it should be the same on Temporal.

@sffc
Copy link
Collaborator

sffc commented Aug 16, 2020

BTW, my intent was that round would only be available on types that already had math. So not YearMonth and MonthDay. Proposal is now updated (1.1.1) to clarify this.

YearMonth has arithmetic. MonthDay doesn't have it.

Also, I realized that no Temporal time scale type has weeks as a concept; weeks are only a concept in Duration. Therefore, it doesn't make any sense to have week rounding on the time scale types.

@justingrant
Copy link
Collaborator Author

BTW, my intent was that round would only be available on types that already had math. So not YearMonth and MonthDay. Proposal is now updated (1.1.1) to clarify this.

YearMonth has arithmetic. MonthDay doesn't have it.

Oops. Fixed. Thanks for catching.

Also, I realized that no Temporal time scale type has weeks as a concept; weeks are only a concept in Duration. Therefore, it doesn't make any sense to have week rounding on the time scale types.

The current proposal already has smallestUnit: 'week' unsupported in round() and toString(). The only remaining use of week is in difference. Do you mean we should remove week as a unit that's emitted by difference()? Or that it doesn't emit weeks today and we shouldn't start?

@sffc
Copy link
Collaborator

sffc commented Aug 20, 2020

The current proposal already has smallestUnit: 'week' unsupported in round() and toString(). The only remaining use of week is in difference. Do you mean we should remove week as a unit that's emitted by difference()? Or that it doesn't emit weeks today and we shouldn't start?

As I've said before, I think there are two types of rounding options, which are similar but should not be seen as the same: time scale rounding, and duration rounding. .difference() is in the duration category, so it should accept weeks. Temporal.DateTime.round is in the time scale category, so it should not accept weeks.

@justingrant
Copy link
Collaborator Author

As I've said before, I think there are two types of rounding options, which are similar but should not be seen as the same: time scale rounding, and duration rounding.

This distinction makes a lot of sense, and I agree with your terms and definitions.

.difference() is in the duration category, so it should accept weeks. Temporal.DateTime.round is in the time scale category, so it should not accept weeks.

OK makes sense. The current proposal includes that round({smallestUnit: 'week'}) and toString({smallestUnit: 'week'}) should throw while difference(other, {largestUnit: 'weeks'}) should not. That's the behavior you're recommending, correct?

@sffc
Copy link
Collaborator

sffc commented Aug 20, 2020

The current proposal includes that round({smallestUnit: 'week'}) and toString({smallestUnit: 'week'}) should throw while difference(other, {largestUnit: 'weeks'}) should not. That's the behavior you're recommending, correct?

Basically yes. I'm suggesting that "week"/"weeks" be rejected if passed to smallestUnit when time scale rounding is being performed, which are all of the methods on non-Duration types except for .difference().


Random idea: since we're adding rounding functionality to existing functions like .difference(), .plus(), .from(), etc., why not add it to .with() and make that the entrypoint instead of .round()?

const now15 = Temporal.now.localDateTime("iso8601").with({ smallestUnit: "minutes", roundingIncrement: 15 });

@sffc
Copy link
Collaborator

sffc commented Aug 20, 2020

1.1: Within the context of time scale units (Date, DateTime, Time, LocalDateTime, YearMonth, MonthDay), I don't find a round method to be compelling with smallestUnit larger than days.

I don't feel strongly about this one way or the other. I've already assumed that 'weeks' is out of scope (and now also 'era' and custom fields) so adding 'months' and 'years' to the excluded list isn't a huge deal because the workarounds are easy.

Right, and I would go further to say that we explicitly don't support the rounding method or options on types on units without a time component (Date and YearMonth).

If we don't support those units fo rounding, then we should probably document a workaround, for example:

I don't know of use cases where programmers want "month rounding". More often, they want "1st of the month" or something like that. We can call that "month rounding", but I don't think that's the term people would look for in documentation.

@justingrant
Copy link
Collaborator Author

@sffc - I'm open to your overall point which is that non-time rounding is much less useful than time rounding. The main use-case I had in mind for month rounding is dealing with quarters (3-month periods), e.g. first day of this quarter / first day of the next quarter.

Perhaps we should get feedback from others too? I don't have a strong opinion either way.

@sffc
Copy link
Collaborator

sffc commented Aug 20, 2020

The main use-case I had in mind for month rounding is dealing with quarters (3-month periods), e.g. first day of this quarter / first day of the next quarter.

Quarters are really complicated. They change not only on calendar system but also by region. See tc39/ecma402#345 (comment)

@justingrant
Copy link
Collaborator Author

Random idea: since we're adding rounding functionality to existing functions like .difference(), .plus(), .from(), etc., why not add it to .with() and make that the entrypoint instead of .round()?

const now15 = Temporal.now.localDateTime("iso8601").with({ smallestUnit: "minutes", roundingIncrement: 15 });

IMHO it seems weird to have units and options cohabitating in the same property bag. What would happen if I did this?

const now15 = Temporal.now.localDateTime("iso8601").with({ 
  minute: 20, 
  second: 30, 
  smallestUnit: "minute", 
  roundingIncrement: 15 
});

Another minor issue would be potential name conflicts between non-ISO-calendar custom fields. In practice this would probably never happen, so not sure how much of a concern this would be.

Also, from an IDE discoverability standpoint, a method called "round" is probably much easier to discover than some options buried in with.

@justingrant
Copy link
Collaborator Author

The main use-case I had in mind for month rounding is dealing with quarters (3-month periods), e.g. first day of this quarter / first day of the next quarter.

Quarters are really complicated. They change not only on calendar system but also by region. See tc39/ecma402#345 (comment)

Yep, agreed. Because of this complexity (not only by calendar and region, but also by use case within each region-- e.g. different companies' fiscal years start in different months) I don't think that a 'quarter' unit should be provided in Temporal or Intl.

That said, 3-month calendar quarters in ISO-calendar-using cases is still quite common.

I still don't have a strong opinion on month/year rounding. If other folks want to remove it, I'd be fine with that but would like to hear more feedback if possible.

@sffc
Copy link
Collaborator

sffc commented Aug 20, 2020

ISO-8601-2 defines a number of year divisions: quarters, seasons, trimesters, etc. If we start supporting rounding to the nearest quarter, we should consider extending that support to all of the year divisions in ISO-8601-2. I don't think we should do this, and therefore I don't think we should have quarter rounding.

@justingrant
Copy link
Collaborator Author

I think we're agreeing? I'm suggesting that if we support month rounding, then the most common quarter case (what 3-month-ISO-calendar-quarter am I in?) will be easier without Temporal having to offer explicitly support 'quarter' as a unit, or any other intra-year-non-month units (seasons, etc.).

@justingrant
Copy link
Collaborator Author

The proposal is now updated with a list of open issues remaining after discussions in the comments. Let me know if I missed any.

Many thanks to @sffc and @gilmoreorless for the reviews.

@ptomato, @pipobscure, @gibson042, @ryzokuken - if you guys have a minute, it'd be great to get your feedback on this proposal. Thanks!

@ptomato ptomato added this to the Stable API milestone Aug 27, 2020
@ptomato
Copy link
Collaborator

ptomato commented Aug 28, 2020

Overall this seems good.

Reading the discussion I've been convinced by Shane's arguments against year and month rounding. I think it's too big of a can of worms, it's weird that day: 15 rounds differently depending on the month, and it can always be added later.

I wasn't in favour of rounding options in toString() since, as mentioned, you can just do .round().toString(), but I can see that there is a use case for outputting an ISO string with a certain precision. However, I do see this as distinct from rounding. Maybe we can solve the problem separately, for example have only smallestUnit in toString(), or a precision option, and not the full complement of rounding options. I'd find it weird to be able to round to 15-minute increments in toString().

Here are my suggestions for the open questions:

  • Should we support rounding of months and years units? (see discussion in comments) Note that weeks and era are already not supported (see -- only months/years are in question.

No (see above).

  • What should be the default rounding mode? Half up? Half even?

Whatever is consistent with Math.

  • Should with() have rounding options? (see discussion in comments)

No, this makes with() too busy and isn't very discoverable. I learned my lesson when I was advocating for with() to balance durations 😄

  • How should roundingIncrement handle cases where the increment isn't an exact divisor of the next-largest unit?

No opinion.

  • Related: how should roundingIncrement handle cases where the next-largest unit has a variable size? (e.g. month length in days, year length in months for non-ISO calendars)

N/A if we don't have year and month rounding.

  • Is it OK that toString includes rounding options?

No (see above), at least not the full complement, but there does need to be some sort of solution for #329.

  • Is it OK that toString controls decimal precision of sub-second units (and whether to show seconds at all) based on smallestUnit?

Yes (but I think this should be pushed to #329 and considered out of scope for this issue)

  • Should toLocaleString accept the same rounding options as toString()?

No, I don't think it should have any. Intl.DateTimeFormat doesn't display any sub-second units anyway, and you control the precision with the options, e.g. date.toLocaleString('en', {hour: 'numeric', minute: 'numeric'}) for minute precision.

Sure.

  • Because the default (see below) for smallestUnit is the smallest unit that the type supports, calling round() with no parameters will return a clone of the same instance but won't change the value. Is this OK?

It's slightly odd but I don't see anything wrong with it. I certainly can't think of a better default.

@sffc
Copy link
Collaborator

sffc commented Aug 28, 2020

Intl.DateTimeFormat doesn't display any sub-second units anyway

It does as of tc39/ecma402#347, and that feature is shipped in Chrome 84. { fractionalSecondDigits: 3 } to display milliseconds.

calling round() with no parameters will return a clone of the same instance

At first I thought I was in favor of not cloning unnecessarily, but my mind has been changed from arguments by Daniel and others, so now I think these methods should always return new objects, even if the objects are the same.

@justingrant
Copy link
Collaborator Author

This proposal is updated to reflect decisions from 2020-08-28 meeting.

@gilmoreorless
Copy link
Contributor

@justingrant I was about to ask for clarification on one of the decisions. But then I realised that an example had been added further down — it just took me a while to see because there's no record of what changed in your edit.

Can I suggest that future proposals of this size use PRs instead of multiple edits to an issue description? Even if it's adding a new document that's not intended to be actually merged, it would bring the following benefits:

  • PRs have a change history, issue description edits don't.
  • Edits to the proposal via commits notify watchers, issue description edits don't.
  • It's easier to follow the chain of comments and replies when looking back on old proposals. For example, @sffc's first comment refers to points which have since been either re-worded or re-numbered. (At least, I think that's what happened, but I can't be sure due to lack of edit history.)

@gibson042
Copy link
Collaborator

@gilmoreorless You can view edit history; the "edited" link in the description header opens a menu.

@gilmoreorless
Copy link
Contributor

@gibson042 Thanks, I had no idea that was clickable! (That's a terrible piece of feature discovery in a UI.)

That solves one of the problems, but it's a clunky implementation. It's still hard to match comments to the proposal text as it was at the time of the comment.

@ptomato ptomato self-assigned this Aug 31, 2020
@ptomato
Copy link
Collaborator

ptomato commented Sep 1, 2020

I've started to work on an implementation of this and I think I've stumbled upon a few things that should be different in the proposal. I'll quote them here so they can be edited later but my comment here will still make sense.

2.1 The smallestUnit property defines where rounding happens. All smaller time units will be zero in the result, and all smaller date units should default to 1.

Second sentence should be "All smaller units will be zero in the result." after dropping date rounding.

2.8 For other types, the smallestUnit can be as large as the largest time unit supported by the type, which will be 'years' for DateTime and LocalDateTime and 'hours' for Time.

Change to "For Time, the smallestUnit is the largest time unit supported by the type, 'hours'. For DateTime and LocalDateTime, 'days' is supported as well."?

6.3 This option will be limited to exact divisors of the next-largest unit. For example, if smallestUnit is 'minute', then roundingIncrement could be either 1 (default), 2, 3, 4, 5, 6, 10, 12, 15, 20, or 30. Others must throw a RangeError.

60 in this case is also an exact divisor of the next-largest unit. I think it's friendly to not throw on, e.g. 60 seconds, although I think it's fine to throw on 120 seconds.

6.7 Absolute has a few differences from behavior of other types
6.7.3 All Absolute rounding increments must divide evenly into one 24-hour day (86400 seconds).

Is this meant to apply to 'minutes', or all smallest units? Can you have { smallestUnit: 'millisecond', roundingIncrement: 7200e3 }) and round to a 2-hour increment? It seems to me that this is fine. (For example, I'd reach instinctively for "86400 seconds" if I knew that "1 day" was not allowed in Absolute rounding, because that's the number that I've memorized, but I'd have to open my calculator to find out the correct number if I had to express it in minutes.)

6.8 ...

What are the allowed values for roundingIncrement if smallestUnit is 'days', for DateTime and LocalDateTime? I'd say only "1" should be allowed, for the same reason we don't support date units rounding; if you have an increment of 2 days then where is it counted from?

@justingrant
Copy link
Collaborator Author

@ptomato I agreed in Friday's meeting to review the issue text to make sure it was still up to date with the implementation.

This was really helpful, especially the sample code for use cases. Thanks for doing this! Proposal text is updated.

ptomato added a commit that referenced this issue Sep 15, 2020
Includes a new abstract operation ToSmallestTemporalDurationUnit. This
is distinct from ToSmallestTemporalUnit because it prefers plural unit
names and does have a fallback value.

Also includes a new abstract operation ValidateTemporalDifferenceUnits.
This makes sure that largestUnit is not smaller than smallestUnit.

Both of these will be reused for Temporal.DateTime.difference and
Temporal.Time.difference in subsequent commits.

See: #827
ptomato added a commit that referenced this issue Sep 15, 2020
Includes a new abstract operation RoundDuration, which will be used in
the other types' difference() methods and Temporal.Duration.round as well.

See: #827
ptomato added a commit that referenced this issue Sep 15, 2020
ptomato added a commit that referenced this issue Sep 15, 2020
Includes a new abstract operation RoundDuration, which will be used in
the other types' difference() methods and Temporal.Duration.round as well.

See: #827
ptomato added a commit that referenced this issue Sep 15, 2020
ptomato added a commit that referenced this issue Sep 16, 2020
Includes several new abstract operations:

- ToSmallestTemporalDurationUnit, which is distinct from
ToSmallestTemporalUnit because it prefers plural unit names and does have
a fallback value;

- ValidateTemporalDifferenceUnits, which makes sure that largestUnit is
not smaller than smallestUnit;

- and MaximumTemporalDurationRoundingIncrement, which computes the maximum
(not inclusive) value for roundingIncrement given the value of
smallestUnit, for rounding a Temporal.Duration.

All of these will be reused for Temporal.DateTime.difference and
Temporal.Time.difference in subsequent commits.

See: #827
ptomato added a commit that referenced this issue Sep 16, 2020
Includes a new abstract operation RoundDuration, which will be used in
the other types' difference() methods and Temporal.Duration.round as well.

See: #827
ptomato added a commit that referenced this issue Sep 16, 2020
ptomato added a commit that referenced this issue Sep 16, 2020
Includes several new abstract operations:

- ToSmallestTemporalDurationUnit, which is distinct from
ToSmallestTemporalUnit because it prefers plural unit names and does have
a fallback value;

- ValidateTemporalDifferenceUnits, which makes sure that largestUnit is
not smaller than smallestUnit;

- and MaximumTemporalDurationRoundingIncrement, which computes the maximum
(not inclusive) value for roundingIncrement given the value of
smallestUnit, for rounding a Temporal.Duration.

All of these will be reused for Temporal.DateTime.difference and
Temporal.Time.difference in subsequent commits.

See: #827
ptomato added a commit that referenced this issue Sep 16, 2020
Includes a new abstract operation RoundDuration, which will be used in
the other types' difference() methods and Temporal.Duration.round as well.

See: #827
ptomato added a commit that referenced this issue Sep 16, 2020
ptomato added a commit that referenced this issue Sep 16, 2020
Includes several new abstract operations:

- ToSmallestTemporalDurationUnit, which is distinct from
ToSmallestTemporalUnit because it prefers plural unit names and does have
a fallback value;

- ValidateTemporalDifferenceUnits, which makes sure that largestUnit is
not smaller than smallestUnit;

- and MaximumTemporalDurationRoundingIncrement, which computes the maximum
(not inclusive) value for roundingIncrement given the value of
smallestUnit, for rounding a Temporal.Duration.

All of these will be reused for Temporal.DateTime.difference and
Temporal.Time.difference in subsequent commits.

See: #827
ptomato added a commit that referenced this issue Sep 16, 2020
Includes a new abstract operation RoundDuration, which will be used in
the other types' difference() methods and Temporal.Duration.round as well.

See: #827
ptomato added a commit that referenced this issue Sep 16, 2020
@ptomato
Copy link
Collaborator

ptomato commented Sep 17, 2020

This landed yesterday but the issue didn't close. Follow ups in #908.

@ptomato ptomato closed this as completed Sep 17, 2020
ptomato added a commit that referenced this issue Dec 3, 2021
To bring the default for largestUnit in PlainDate's since() and until()
methods in line with other types' since() and until() methods, we have to
add an algorithm step for LargerOfTwoTemporalUnits.

Without this, code such as
date1.until(date2, { smallestUnit: 'months' })
would throw because the default largestUnit is days. As per
#827 (comment)
this was not intended.

PlainDate seems to be the only place where this was not working as
intended. ZonedDateTime, Instant, PlainTime, PlainYearMonth, and
PlainDateTime either already have this step or their default largestUnit
is already the largest one so they wouldn't have this problem.

The reference polyfill code is already correct in this regard.

Closes: #1864
ptomato added a commit that referenced this issue Dec 17, 2021
To bring the default for largestUnit in PlainDate's since() and until()
methods in line with other types' since() and until() methods, we have to
add an algorithm step for LargerOfTwoTemporalUnits.

Without this, code such as
date1.until(date2, { smallestUnit: 'months' })
would throw because the default largestUnit is days. As per
#827 (comment)
this was not intended.

PlainDate seems to be the only place where this was not working as
intended. ZonedDateTime, Instant, PlainTime, PlainYearMonth, and
PlainDateTime either already have this step or their default largestUnit
is already the largest one so they wouldn't have this problem.

The reference polyfill code is already correct in this regard.

Closes: #1864
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

6 participants