Inconsistent results in DifferenceISODate? #2535
Comments
|
You raise an interesting point. There are essentially two options for balancing a duration: a. Only add a larger unit if the larger unit can be added without exceeding the starting date, after clamping the values of smaller units according to the default @ptomato I suspect that we probably discussed this tradeoff many years ago. Do you remember the reasoning for choosing one over the other? It looks like currently we're doing (a) to calculate from 1970-01-31 to 1970-02-28: Using the alternative algorithm (b) would look like this: It'd be useful to understand how these two algorithmic choices would affect two other similar cases:
// DST skipped the hour 2:00-3:00 on 2020-03-08 in this time zone
Temporal.ZonedDateTime.from('2020-03-09T02:30-07:00[America/Los_Angeles]')
.since('2020-03-08T03:30-07:00[America/Los_Angeles]', { largestUnit: 'day' });
// => P1D
// 2001 is a leap year with leap month M04L. 2000 is not a leap year.
Temporal.PlainDate.from({ year: 2001, monthCode: 'M04L', day: 1, calendar: 'chinese' })
.until({ year: 2002, monthCode: 'M04', day: 1, calendar: 'chinese' }, { largestUnit: 'year' })
// => P12MMy initial opinion before digging into this too deeply is that it'd be good for all three of these cases to behave consistently. I don't yet have an opinion about whether (a) or (b) is the "more correct" behavior. |
gibson042
added
spec-text
|
Meeting 2023-04-13: Let's research why we originally picked the current behavior, and once we know that history then we can figure out if the existing behavior should be changed, or if the original reason is compelling enough to deviate from Java behavior. |
|
Related issues:
We can probably pull up discussions in more detail in the meeting notes, but I haven't looked for those yet. What I read in those various issue threads, consistently, is that we've considered this behaviour correct, and this is particularly important to @pipobscure: const posDuration = earlier.until(later);
const negDuration = later.until(earlier);
earlier.add(posDuration).equals(later);
later.add(negDuration).equals(earlier);
earlier.subtract(negDuration).equals(later);
later.subtract(posDuration).equals(earlier); |
|
Summary of discussion on 2023-04-27: @pipobscure's statement of commutativity is concise: A + D = B ⇔ B - D = A In other words, for two dates The example in the OP is one where commutativity does not hold, because of the default However, the inconsistent behaviour for the 27th versus the 28th of the month looks suspicious. It might just be a bug, in the first place. We still need to research this. |
|
Unless I'm misinterpreting, commutativity would hold if all the cases where start day-of-month is greater than end day-of-month would return a duration in which the largest unit is days, as seems to be the case for Java. I think the culprit in Temporal's divergence is the inherent privileging of last-day-of-month by the embedded "constrain" in step 1.k of DifferenceISODate (and presumably also in step 1.n.iii for other similar cases). |
|
I'm finally done researching this. Sorry for delays. TL;DR - I think the current spec is correct, given the criteria used when we designed the algorithm about 3 years ago. Some relevant meeting discussion is here: https://github.com/tc39/proposal-temporal/blob/main/meetings/agenda-minutes-2020-10-15.md#anything-left-to-discuss-on-order-of-operations-993--913 The background for decisions in this issue is best captured here: #993 (comment). That comment is still a bit out of date (it refers to an older operation
Regardless of how this issue is resolved, this invariant cannot hold for all values of A, B, and D. This is because a = Temporal.PlainDate.from('2023-01-31')
d = Temporal.Duration.from({months: 1});
b = a.add(d); // => 2020-02-28 (constrained!)
b.subtract(d); // => 2020-01-28
a.equals(b.subtract(d)); // => falseHowever, different invariants should hold in the current design:
Assuming that my memory is correct, making these invariants work and enabling reversibility was the reason why we wrote the DifferenceISODate algorithm like we did. There may have been other reasons too. So I don't think that a spec change is needed, unless there's some new information that would lead us to de-prioritize reversibility in favor of some other reason (e.g. Java compatibility) or unless the reversible invariants above don't hold with the current spec. I'm not saying that the current spec yields perfect results. The results shown in the OP are indeed confusing. But I think the high-level point is that all results are potentially confusing, depending on what the user's expectations are and what use cases are being targeted. There is no perfect solution here, only less-bad solutions for some users and some use cases. So the approach we took in Temporal was to: a) optimize the algorithm for reversibility, and b) set the default Feel free to push back if I got anything wrong above! |
|
That's great news. I read through the discussion and what you described (optimizing for reversibility and defaulting |
These invariants don't hold with the current spec. For example: Script to test the invariants: function* dates() {
for (let y = 1972; y <= 1973; ++y) {
for (let m = 1; m <= 12; ++m) {
for (let d = 1; d <= 31; ++d) {
try {
yield new Temporal.PlainDate(y, m, d);
} catch {}
}
}
}
}
function assertEq(a, e, m) {
if (a !== e) console.log(`${m}: ${a} != ${e}`);
}
for (let a of dates()) {
for (let b of dates()) {
let d = a.until(b, {largestUnit: "month"});
let e = b.subtract(d);
assertEq(e.toString(), a.toString(), `a=${a}, b=${b}, d=${d}, e=${e}`);
}
} |
|
The issue description clearly demonstrates a case where currently specified behavior is less useful than a particular alternative, with the alternative actually being more reversible. In what cases is there a compensating benefit—and if none can be identified, then what would discourage switching to what seems like a strictly superior approach? This is advertised as a fundamental reason to introduce Temporal, so getting it wrong would undermine quite a bit and approach catastrophe. |
|
During discussion today, I came to the conclusion that |
|
Meeting 2023-10-12:
|
@gibson042 Would this requirement ever lead to durations with unexpectedly large unit values? I'm not saying it would, only wondering if there are edge cases where results like P1M32D could happen. |
|
I don't think that would be possible in the ISO 8601 calendar, because the 32D part would always be equivalent to 1MnD where n is small, and therefore the whole duration would instead be P2MnD. But it would be possible in a calendar with two consecutive short months—for example, if consecutive months W, X, Y, and Z respectively had 31, 31, 30, and 30 days, then |
|
Here are the libraries/platforms that I looked into. tl;dr The majority of them don't even support this kind of calculation with largestUnit months. Temporal is part of a small minority that do. Other JS libraries match Temporal's behaviour. Java and ICU4X are different from Temporal and different from each other. JavaScript
Python
C
C++
C#
Rust
Java
(Note, I'm not too familiar with most of these libraries so I may well have missed some capabilities. Please let me know if I came to the wrong conclusions. Also let me know if I missed one that you think I should look into.)
Moment and Luxon behave the same as Temporal. I found an old PR suggesting that they struggled with "buggy month/year diffs" and that would at least suggest that this behaviour is intentional. Maybe @maggiepint or @mattjohnsonpint might know more about this. The one difference is that Moment returns 0 months when asking for a difference in months where Temporal and Luxon return a number of days instead, giving the "0 months" entries in the bottom of the table. This makes sense because they don't have a duration type, it's just a number. Date arithmetic with I found an interesting discussion on Reddit while I was searching for Rust datetime libraries. My takeaway from there is that this could be unintuitive either way depending on your application. My conclusion is that there isn't one obviously correct behaviour here, and that we may as well match other JS libraries by keeping the status quo, which wasn't arbitrarily chosen. Code snippetsTemporalfor (const base of [28, 27]) {
let end = new Temporal.PlainDate(1970, 2, base);
for (const startDay of [base, base + 1, base + 2, base + 3]) {
for (const largestUnit of ['months', 'days']) {
let start = new Temporal.PlainDate(1970, 1, startDay);
const result = start.until(end, { largestUnit });
console.log(`${start} .. ${end} = ${result} in ${largestUnit}`);
}
}
}Momentimport moment from 'moment';
for (const base of [28, 27]) {
let end = moment.utc([1970, 1, base]);
for (const startDay of [base, base + 1, base + 2, base + 3]) {
for (const largestUnit of ['months', 'days']) {
let start = moment.utc([1970, 0, startDay]);
const result = end.diff(start, largestUnit);
console.log(`${start.format('YYYY-MM-DD')} .. ${end.format('YYYY-MM-DD')} = ${result} in ${largestUnit}`);
}
}
}Luxonimport {DateTime} from 'luxon';
for (const base of [28, 27]) {
let end = DateTime.utc(1970, 2, base);
for (const startDay of [base, base + 1, base + 2, base + 3]) {
for (const largestUnit of [['months', 'days'], 'days']) {
let start = DateTime.utc(1970, 1, startDay);
const result = end.diff(start, largestUnit);
console.log(`${start.toISODate()} .. ${end.toISODate()} = ${result.toHuman()} in ${largestUnit}`);
}
}
}ICU4Xuse icu::calendar::{Date, DateDurationUnit};
use icu::datetime::{options::length, DateFormatter};
use icu::locid::locale;
fn main() {
for base in (27..=28).rev() {
let formatter =
DateFormatter::try_new_with_length(&locale!("en-CA").into(), length::Date::Short)
.unwrap();
let end = Date::try_new_iso_date(1970, 2, base).unwrap();
let end_str = formatter.format_to_string(&end.to_any()).unwrap();
for start_day in base..base + 4 {
let start = Date::try_new_iso_date(1970, 1, start_day).unwrap();
// Note: until() is unstable. Arguments are reversed
let result_in_months =
end.until(&start, DateDurationUnit::Months, DateDurationUnit::Days);
let start_str = formatter.format_to_string(&start.to_any()).unwrap();
println!(
"{} .. {} = {} m {} d in months",
start_str, end_str, result_in_months.months, result_in_months.days
);
let result_in_days = end.until(&start, DateDurationUnit::Days, DateDurationUnit::Days);
println!(
"{} .. {} = {} m {} d in days",
start_str, end_str, result_in_days.months, result_in_months.days
);
}
}
}java.timeimport java.time.LocalDate;
import java.time.Period;
import java.time.temporal.ChronoUnit;
import java.util.stream.IntStream;
class Application {
public static void main(String[] args) {
for (int base : new int[] { 28, 27 }) {
LocalDate end = LocalDate.of(1970, 2, base);
for (int startDay : IntStream.range(base, base + 4).toArray()) {
LocalDate start = LocalDate.of(1970, 1, startDay);
Period result = start.until(end);
long monthResult = start.until(end, ChronoUnit.MONTHS);
System.out.printf("%s .. %s = %d in months (%s)\n", start, end, monthResult, result);
long dayResult = start.until(end, ChronoUnit.DAYS);
System.out.printf("%s .. %s = %d in days\n", start, end, dayResult);
}
}
}
} |
I did some basic research to validate that the invariants above:
I found that our tests confirm those invariants: proposal-temporal/polyfill/test/datemath.mjs Lines 79 to 90 in ee2ddeb My conclusions so far:
|
|
Meeting 2023-10-26:
|
|
Can someone explain to me why we don't just use the following algorithm: Assumptions for illustration purposes:
Algorithm for
Code: https://jsbin.com/zunaliriki/1/edit?js,console function DifferenceISODate(y1, m1, d1, y2, m2, d2, largestUnit) {
console.assert(
largestUnit === "years" ||
largestUnit === "months" ||
largestUnit === "days"
);
if (largestUnit === "days") {
return DaysBetweenISO(y1, m1, d1, y2, m2, d2);
}
let yd = y2 - y1;
let md = m2 - m1;
let dd = d2 - d1;
console.assert(yd >= 0); // because date 1 < date 2
if (largestUnit === "months" && yd > 0) {
md += yd * 12;
yd = 0;
}
if (dd < 0) {
md -= 1;
dd += DaysBetweenISO(y2, m2 - 1, d2, y2, m2, d2);
}
if (md < 0) {
yd -= 1;
md += 12;
}
console.assert(yd >= 0);
console.assert(md >= 0);
console.assert(dd >= 0);
console.log(`${y1}-${m1}-${d1} to ${y2}-${m2}-${d2}: ${yd}y ${md}m ${dd}d (${largestUnit})`);
}
|
That might actually be equivalent to the algorithm we use in the case where (y1, m1, d1) < (y2, m2, d2), but it does actually matter how we generalize it to the other way around. For example earlier = Temporal.PlainDate.from('2023-03-25')
later = Temporal.PlainDate.from('2023-07-05')
earlier.until(later, {largestUnit: 'months'})This is 3 months, 10 days according to both the current DifferenceISODate and the algorithm from the above comment. Arithmetic is relative to the receiver, so to verify the invariant we add 3 months to 2023-03-25, getting 2023-06-25, then we add 10 days to 2023-06-25, getting 2023-07-05. later.since(earlier, {largestUnit: 'months'})This is 3 months, 11 days according to the current DifferenceISODate. To verify the invariant we subtract 3 months from 2023-07-05, getting 2023-04-05, then we subtract 11 days from 2023-04-05, getting 2023-03-25. We'd need to decide how to handle this case with the above algorithm, and doing so may well give an algorithm that's equivalent to the current one. |
Here's what I've managed to come up with.
(Preferably I wanted to include something about how with |
OK, I think I've got it. This should really be subject to near-exhaustive testing of every date in a four-year cycle to every other, but I have not done so here. Expressed as JavaScript: const DifferenceISODate = (y1, m1, d1, y2, m2, d2, largestUnit = "year") => {
if (largestUnit === "year" || largestUnit === "month") {
const sign = (y2 < y1) || (y2 === y1 && m2 < m1) || (y2 === y1 && m2 === m1 && d2 < d1) ? -1 : 1;
if (sign < 0) {
[y1, m1, d1, y2, m2, d2] = [y2, m2, d2, y1, m1, d1];
}
assert((y1 < y2) || (y1 === y2 && m1 < m2) || (y1 === y2 && m1 === m2 && d1 <= d2));
let years = y2 - y1;
if (m2 < m1 || (m2 === m1 && d2 < d1)) years--;
let months = (m2 - m1 + 12) % 12;
let days = d2 - d1;
if (days < 0) {
let m = m2;
do {
m = (m - 1) || 12;
months--;
days += ISODaysInMonth(y2, m);
} while (d1 > ISODaysInMonth(y2, m));
while (months < 0) {
years--;
months += 12;
}
}
if (largestUnit === "month") {
months += years * 12;
years = 0;
}
if (sign < 0) [years, months, days] = [-years, -months, -days];
return { years, months, days };
} else {
assert(largestUnit === "day" || largestUnit === "week");
// ...
}
};
const pairs = [
...[27, 28, 29, 30, 31].map(day => [`1970-01-${day}`, "1970-02-27"]),
...[27, 28, 29, 30, 31].map(day => [`1970-01-${day}`, "1970-02-28"]),
["1970-01-02", "1970-03-01"],
["1972-01-02", "1972-03-01"],
["1971-12-02", "1972-03-01"],
["1971-12-30", "1972-03-01"],
["1971-12-30", "1972-03-10"],
["1969-12-30", "1972-03-10"],
];
for(const [start, end] of pairs) {
const [[y1, m1, d1], [y2, m2, d2]] = [start, end].map(ymd =>
ymd.match(/^0*(\d+)-0*(\d+)-0*(\d+)$/).slice(1).map(x => +x)
);
console.log(`${start} to ${end}`, DifferenceISODate(y1, m1, d1, y2, m2, d2, "year"));
}
/* =>
1970-01-27 to 1970-02-27 { years: 0, months: 1, days: 0 }
1970-01-28 to 1970-02-27 { years: 0, months: 0, days: 30 }
1970-01-29 to 1970-02-27 { years: 0, months: 0, days: 29 }
1970-01-30 to 1970-02-27 { years: 0, months: 0, days: 28 }
1970-01-31 to 1970-02-27 { years: 0, months: 0, days: 27 }
1970-01-27 to 1970-02-28 { years: 0, months: 1, days: 1 }
1970-01-28 to 1970-02-28 { years: 0, months: 1, days: 0 }
1970-01-29 to 1970-02-28 { years: 0, months: 0, days: 30 }
1970-01-30 to 1970-02-28 { years: 0, months: 0, days: 29 }
1970-01-31 to 1970-02-28 { years: 0, months: 0, days: 28 }
1970-01-02 to 1970-03-01 { years: 0, months: 1, days: 27 }
1972-01-02 to 1972-03-01 { years: 0, months: 1, days: 28 }
1971-12-02 to 1972-03-01 { years: 0, months: 2, days: 28 }
1971-12-30 to 1972-03-01 { years: 0, months: 1, days: 31 }
1971-12-30 to 1972-03-10 { years: 0, months: 1, days: 40 }
1969-12-30 to 1972-03-10 { years: 2, months: 1, days: 40 }
*/ |
I wrote a script to do this, and found some differences that I think must be unintentional. There seem to be some date pairs where the result is off by one year. Here are the first few: Here's the scriptimport { strict as assert } from 'assert';
import fs from 'node:fs';
import ProgressBar from 'progress';
import {
BalanceISODate,
DifferenceISODate as diffOld,
ISODateTimePartString,
ISODaysInMonth
} from './lib/ecmascript.mjs';
function diffNew(y1, m1, d1, y2, m2, d2, largestUnit) {
if (largestUnit === 'year' || largestUnit === 'month') {
const sign = y2 < y1 || (y2 === y1 && m2 < m1) || (y2 === y1 && m2 === m1 && d2 < d1) ? -1 : 1;
if (sign < 0) {
[y1, m1, d1, y2, m2, d2] = [y2, m2, d2, y1, m1, d1];
}
assert(y1 < y2 || (y1 === y2 && m1 < m2) || (y1 === y2 && m1 === m2 && d1 <= d2));
let years = y2 - y1;
if (m2 < m1 || (m2 === m1 && d2 < d1)) years--;
let months = (m2 - m1 + 12) % 12;
let days = d2 - d1;
if (days < 0) {
let m = m2;
do {
m = m - 1 || 12;
months--;
days += ISODaysInMonth(y2, m);
} while (d1 > ISODaysInMonth(y2, m));
while (months < 0) {
years--;
months += 12;
}
}
if (largestUnit === 'month') {
months += years * 12;
years = 0;
}
if (sign < 0) [years, months, days] = [-years, -months, -days];
return { years, months, days };
} else {
assert(largestUnit === 'day' || largestUnit === 'week');
// ...
}
}
function fdate(y, m, d) {
return `${y}-${ISODateTimePartString(m)}-${ISODateTimePartString(d)}`;
}
function fdur({ years, months, days }) {
let result = '';
if (years) result += `${years} y, `;
if (months) result += `${months} m, `;
result += `${days} d`;
return result;
}
const progress = new ProgressBar(':bar :percent (:current/:total) | :etas', {
total: 1461 ** 2,
complete: '\u2588',
incomplete: '\u2591',
width: 20,
stream: process.stdout,
renderThrottle: 50,
clear: true
});
const fd = fs.openSync('./exhaust.txt', 'w');
let sameCount = 0;
let differentCount = 0;
for (let i = 0; i <= 1461; i++) {
const { year: y1, month: m1, day: d1 } = BalanceISODate(1970, 1, i + 1);
for (let j = 0; j <= 1461; j++) {
const { year: y2, month: m2, day: d2 } = BalanceISODate(1970, 1, j + 1);
for (const largestUnit of ['month', 'year']) {
const old = diffOld(y1, m1, d1, y2, m2, d2, largestUnit);
const nu = diffNew(y1, m1, d1, y2, m2, d2, largestUnit);
if (old.years !== nu.years || old.months !== nu.months || old.days !== nu.days) {
fs.writeSync(
fd,
`${fdate(y1, m1, d1)}..${fdate(y2, m2, d2)} in ${largestUnit}s: old ${fdur(old)}, new ${fdur(nu)}\n`
);
differentCount++;
} else {
sameCount++;
}
}
progress.tick();
}
}
fs.closeSync(fd);
console.log('same', sameCount, 'different', differentCount); |
|
While I was writing exhaustive scripts, I also verified the invariants I proposed, with the status quo algorithm:
Here's the scriptimport { strict as assert } from 'assert';
import ProgressBar from 'progress';
import * as Temporal from './lib/temporal.mjs';
const base = Temporal.PlainDate.from('1970-01-01');
const progress = new ProgressBar(':bar :percent (:current/:total) | :etas', {
total: 1461 ** 2,
complete: '\u2588',
incomplete: '\u2591',
width: 20,
stream: process.stdout,
renderThrottle: 50,
clear: true
});
for (let i = 0; i <= 1461; i++) {
const a = base.add({ days: i });
for (let j = 0; j <= 1461; j++) {
const b = base.add({ days: j });
for (const largestUnit of ['weeks', 'months', 'years']) {
// Invariant: if A.until(B) is D, then A.add(D) is B, for any largestUnit.
{
const d = a.until(b, { largestUnit });
const b2 = a.add(d);
assert.equal(b2.toString(), b.toString(), `${a}.until(${b}, ${largestUnit}) == ${d}, ${a}.add(${d}) == ${b2}`);
}
// Invariant: if A.since(B) is D, then A.subtract(D) is B, for any largestUnit.
{
const d = a.since(b, { largestUnit });
const b2 = a.subtract(d);
assert.equal(
b2.toString(),
b.toString(),
`${a}.since(${b}, ${largestUnit}) == ${d}, ${a}.subtract(${d}) == ${b2}`
);
}
}
{
const d = a.until(b);
const b2 = a.add(d);
assert.equal(b2.toString(), b.toString(), `${a}.until(${b}) == ${d}, ${a}.add(${d}) == ${b2}`);
// Invariant: if A.until(B) is D, then B.subtract(D) is A, for any non-calendar largestUnit.
const a2 = b.subtract(d);
assert.equal(a2.toString(), a.toString(), `${a}.until(${b}) == ${d}, ${b}.subtract(${d}) == ${a2}`);
}
{
const d = a.since(b);
const b2 = a.subtract(d);
assert.equal(b2.toString(), b.toString(), `${a}.since(${b}) == ${d}, ${a}.subtract(${d}) == ${b2}`);
// Invariant: if A.since(B) is D, then B.add(D) is A, for any non-calendar largestUnit.
const a2 = b.add(d);
assert.equal(a2.toString(), a.toString(), `${a}.since(${b}) == ${d}, ${b}.add(${d}) == ${a2}`);
}
progress.tick();
}
} |
Thanks so much! Here is a corrected implementation of the desired algorithm: // S is a sign function that classifies zero as positive.
const S = x => x < 0 ? -1 : 1;
export function diffNew(y1, m1, d1, y2, m2, d2, largestUnit) {
if (largestUnit === 'year' || largestUnit === 'month') {
let years = y2 - y1, months = m2 - m1, days = d2 - d1;
if (years === 0 && months === 0) return { years: 0, months: 0, days };
const sign = S(years || months || days);
if (months !== 0 && S(months) !== sign) {
years -= sign;
months += 12 * sign;
}
assert(sign > 0 && years >= 0 && months >= 0 || sign < 0 && years <= 0 && months <= 0);
let needsFixup = days !== 0 && S(days) !== sign;
if (!needsFixup && sign < 0) {
const newMonth = (m1 + months + 11) % 12 + 1;
const newYear = newMonth <= m1 ? y1 + years : y1 + years - 1;
if (d1 > ISODaysInMonth(newYear, newMonth)) needsFixup = true;
}
if (needsFixup) {
let m = m2;
do {
months -= sign;
if (sign > 0) {
m = (m - 1) || 12;
days += ISODaysInMonth(y2, m);
} else {
days -= ISODaysInMonth(y2, m);
m = (m + 1) % 13 || 1;
}
} while (d1 > ISODaysInMonth(y2, m));
if (months !== 0 && S(months) !== sign) {
years -= sign;
months += 12 * sign;
}
assert(sign > 0 && years >= 0 && months >= 0 || sign < 0 && years <= 0 && months <= 0);
}
if (years !== 0 && largestUnit === 'month') {
months += years * 12;
years = 0;
}
return { years, months, days };
} else {
assert(largestUnit === 'day' || largestUnit === 'week');
// ...
}
}Running this in the difference script produces
|
|
Thanks for the fixup. I've looked over the results as well and agree with that categorization. It hasn't changed my opinion on this issue though. I remain in agreement with Justin, that some of the results that would be changed are problematic. I'll focus on these two which I think are good examples of the most egregious cases: // (a)
Temporal.PlainDate.from('1970-01-29').until('1970-03-28', { largestUnit: 'months' })
// current: 1 month and 28 days
// proposed: 58 days
// (b)
Temporal.PlainDate.from('1970-01-31').until('1971-05-30', { largestUnit: 'years' })
// current: 1 year, 3 months, and 30 days
// proposed: 1 year, 2 months, and 60 daysI think these would be real headscratchers if they showed up in a UI. They seem almost certainly not what I would want as an end user. FWIW, I did a comparison with these dates using Luxon and java.time, and unlike with the original set of dates that Anba brought to our attention, they both agree with Temporal. So apparently java.time uses some other algorithm that doesn't produce these cases. I will see what I can find out about this algorithm in the OpenJDK source code. Meanwhile, here's my preference. Given that:
I'd prefer that we consider this out of scope for Temporal, keeping the door open to introduce a future proposal that adds an option with which callers can opt into the alternative algorithm. |
|
I continue to see results raised in the original post of this issue as very problematic. Even if Temporal doesn't switch to the fully distinct algorithm above, it would probably be better to adopt a middle ground that e.g. limits days to be within the penultimate month from start to end. |
@gibson042 could you give an example of what you mean? |
|
Great discussion! Looking forward to (hopefully!) finally resolving this question tomorrow morning. It seems like there are three somewhat related issues on the table:
Is this correct? I think it might be helpful to consider each of these in sequence to structure the discussion. I'll try to respond in sequence: 1. Where should
|
|
Hi @justingrant, I'll be in the upcoming meeting but wanted to get my thoughts written out first: 1. Where should add constrain intermediate values?
I like this algorithm and yes, PlainDateTime and ZonedDateTime already seem to do this. 2. Where should until constrain intermediate values?I'd like to try to answer this but it's hard given that the algorithm has been described in constraint-based terms in this thread rather than imperatively. Here's my stab at explaining the ideal algorithm imperatively. If it conflicts with that's been said already, apologies! If it's in alignment with what's been said already. Maybe it will serve as the basis for a more human-readable explanation in the docs: Given
3. Why don't round and until always agree?I'll try to discuss in the meeting. |
|
I ran the exhaustive test on the algorithm that we discussed today. Click here for full codeimport fs from 'node:fs';
import ProgressBar from 'progress';
import {
BalanceISODate,
BalanceISOYearMonth,
CompareISODate,
ConstrainISODate,
DifferenceISODate as diffOld,
ISODateTimePartString
} from './lib/ecmascript.mjs';
// “Surpassing” here (and in all steps below) means to take YMD all into
// account lexicographically.
function surpasses(sign, y1, m1, d1, y2, m2, d2) {
if (sign > 0) {
return y1 > y2 || (y1 === y2 && m1 > m2) || (y1 === y2 && m1 === m2 && d1 > d2);
}
return y1 < y2 || (y1 === y2 && m1 < m2) || (y1 === y2 && m1 === m2 && d1 < d2);
}
export function diffNew(y1, m1, d1, y2, m2, d2, largestUnit) {
const sign = -CompareISODate(y1, m1, d1, y2, m2, d2);
if (sign === 0) return { years: 0, months: 0, weeks: 0, days: 0 };
// 1. Add (without constraining) as many years as possible to y1-m1-d1 without
// surpassing y2-m2-d2.
let years = 0;
if (largestUnit === 'year') {
while (!surpasses(sign, y1 + years, m1, d1, y2, m2, d2)) years += sign;
years -= sign;
}
// 2. After we have a result, constrain to a real year/month (NOT
// year/month/day).
// (always a no-op here, only matters for lunisolar calendars)
// 3. Add (without constraining) as many months as possible to y1-m1-d1
// without surpassing y2-m2-d2. (taking all units into account).
let months = 0;
if (largestUnit === 'year' || largestUnit === 'month') {
let y = y1 + years;
let m = m1;
while (!surpasses(sign, y, m, d1, y2, m2, d2)) {
months += sign;
({ year: y, month: m } = BalanceISOYearMonth(y, m + sign));
}
months -= sign;
}
// 4. After we have a result, constrain to a real year/month/day.
const { year: yy, month: my } = BalanceISOYearMonth(y1 + years, m1 + months);
({ year: y1, month: m1, day: d1 } = ConstrainISODate(yy, my, d1));
// 5. If largestUnit=week, add as many weeks as possible to y1-m1-d1 without
// surpassing y2-m2-d2.
let weeks = 0;
if (largestUnit === 'week') {
let y = y1;
let m = m1;
let d = d1;
while (!surpasses(sign, y, m, d, y2, m2, d2)) {
weeks += sign;
({ year: y, month: m, day: d } = BalanceISODate(y, m, d + 7 * sign));
}
weeks -= sign;
}
// 6. Add as many days as possible to y1-m1-d1 without surpassing y2-m2-d2.
let days = 0;
let { year: y, month: m, day: d } = BalanceISODate(y1, m1, d1 + 7 * weeks);
while (!surpasses(sign, y, m, d, y2, m2, d2)) {
days += sign;
({ year: y, month: m, day: d } = BalanceISODate(y, m, d + sign));
}
days -= sign;
return { years, months, weeks, days };
}
function fdate(y, m, d) {
return `${y}-${ISODateTimePartString(m)}-${ISODateTimePartString(d)}`;
}
function fdur({ years, months, weeks, days }) {
let result = '';
if (years) result += `${years} y, `;
if (months) result += `${months} m, `;
if (weeks) result += `${weeks} w, `;
result += `${days} d`;
return result;
}
const progress = new ProgressBar(':bar :percent (:current/:total) | :etas', {
total: 1461 ** 2,
complete: '\u2588',
incomplete: '\u2591',
width: 20,
stream: process.stdout,
renderThrottle: 50,
clear: true
});
const fd = fs.openSync('./exhaust.txt', 'w');
let sameCount = 0;
let differentCount = 0;
for (let i = 0; i <= 1461; i++) {
const { year: y1, month: m1, day: d1 } = BalanceISODate(1970, 1, i + 1);
for (let j = 0; j <= 1461; j++) {
const { year: y2, month: m2, day: d2 } = BalanceISODate(1970, 1, j + 1);
for (const largestUnit of ['month', 'year', 'week', 'day']) {
const old = diffOld(y1, m1, d1, y2, m2, d2, largestUnit);
const nu = diffNew(y1, m1, d1, y2, m2, d2, largestUnit);
if (old.years !== nu.years || old.months !== nu.months || old.weeks !== nu.weeks || old.days !== nu.days) {
fs.writeSync(
fd,
`${fdate(y1, m1, d1)}..${fdate(y2, m2, d2)} in ${largestUnit}s: old ${fdur(old)}; new ${fdur(nu)}\n`
);
differentCount++;
} else {
sameCount++;
}
}
progress.tick();
}
}
fs.closeSync(fd);
// eslint-disable-next-line no-console
console.log('same', sameCount, 'different', differentCount);While this is an inefficient from-scratch implementation where the goal was to stay as faithful as possible to the prose steps we discussed in the meeting, I am pretty sure the results are identical to the "hybrid" algorithm that Richard expressed in an earlier comment: #2535 (comment) The nice thing about this implementation is that you can replace the function surpasses(sign, y1, m1, d1, y2, m2, d2) {
const constrained = ConstrainISODate(y1, m1, d1);
const cmp = -CompareISODate(constrained.year, constrained.month, constrained.day, y2, m2, d2);
return cmp !== 0 && cmp !== sign;
} |
|
Wow @ptomato! I'm sure the majority of future discussion will stay in Calendar-land, but since I won't be in the meeting tomorrow, I wanted to touch on #2535 (comment), which is about when Moving to "(3) Why don't round and until always agree", the final problem I have is expressed here: #2742 (comment) It can be solved if Duration::round follows the same code path as PDT/ZDT::until. Dirty pseudocode: Duration.prototype.round = function({
relativeTo,
smallestUnit,
largestUnit,
roundingIncrement,
}) {
return relativeTo.until(
relativeTo.add(this),
{ smallestUnit, largestUnit, roundingIncrement }
)
}Additional work would be required to make |
|
My previous comment was "opinion-neutral" but I'd like to add this second comment to summarize my position. Let's stick to Shane's nomenclature: "balanced" = current algorithm in the spec text, "exact" = algorithm that returns things like "1 month and 35 days" and is reversible, and "hybrid" = the algorithm referred to in my previous comment that was discussed in today's meeting.
My strong preference would be to keep the "balanced" algorithm. It sounds like we need to generalize the "balanced" algorithm to ZonedDateTime.until/since as well, because the lack of this is causing the buggy behaviour as Adam described in the comment above. I'm in favour of fixing this bug. I would support adding a new option in Temporal V2 to opt in to the "exact" algorithm, but not in scope of this proposal. |
|
@ptomato, I think I'm mixing up the "hybrid" algorithm with the "balanced" algorithm. You said the "balanced" algorithm can now also be explained more intuitively. Where can I find that explanation? |
@arshaw It's also in #2535 (comment) – the part where I mentioned replacing the |
|
Got it, thanks for the clarity @ptomato. And is it possible to express the "exact" algorithm using that "hybrid" code as a base as well? |
|
Meeting 2024-01-25
We agreed that the alrogithm described in (add link) is both what we want and what the status quo is.
I believe that we agreed (correct me if wrong) that we think this is a bug and should fix it.
We're still discussing this one. :-) |
|
Here's the algorithm I noted down in the meeting, based on @arshaw's and modified based on our discussion. @ptomato Is this the "balanced" algorithm? I admit I'm not quite understanding what "hybrid" is here.
|
|
I think there were 3 goals, and not all algorithms satisfy them:
How do the proposed algorithms size up on these three points? |
|
I think it'd be helpful to find common, real-world use cases for each algorithm, to help us pick the least bad Here's a few that I know about: (Note these are only use cases for users who have opted into the non-default The most popular use case for Another is businesses (I'm thinking of car rentals) that sell services that have volume discounts for longer chunks like months. To prepare your bill, they need to know how many months it's been since your start date, and they probably also want to know the leftover days so they can charge you for a partial month. FWIW, neither of these cases would be OK with a result with days > 31. In the formatting case, it'd look weird and buggy. In the latter, the customer would think that the business is trying to cheat them by charging them more than one month's worth of the higher daily price. The other use cases that I'm familiar with for Are there other popular use cases for year or month |
|
Responses to various questions 😄
@arshaw I'm not sure. I was wondering that yesterday as well, but didn't have time to work it out.
@justingrant
I don't know off the top of my head whether the hybrid algorithm produces unique results when you hold one of the dates constant and vary the other. I think not, but I'd have to check. |
I disagree; it is definitely reasonable to render e.g. "35 days ago", and cases where that is not desired call for distinct smallestUnit rounding. |
|
Thinking out loud:
What should X and Y be? |
|
There is no constraining / ambiguous behavior taking place here. I'll choose a different set of dates for the next post. |
|
In this table, there are 6 instances where constraining may need to take place during calculation. Here are correct outcomes for A-F:
|
This adjusts the difference algorithm for Gregorian-year dates so that when an intermediate date occurs past the end of a month, it is not shifted to the end of that month. Previously, in some edge cases where taking the difference in months or years would return a number of months and zero days, we now return one month less and 28, 29, or 30 days instead. Example: 1970-01-29 until 1971-02-28, largestUnit years Old result: 1 year, 1 month New result: 1 year, 30 days Note that largestUnit weeks and largestUnit days, the latter of which is the default, are not affected. TODO: Make the algorithm in the reference code faster, to help show how it could be implemented. Closes: #2535
This adjusts the difference algorithm for Gregorian-year dates so that when an intermediate date occurs past the end of a month, it is not shifted to the end of that month. Previously, in some edge cases where taking the difference in months or years would return a number of months and zero days, we now return one month less and 28, 29, or 30 days instead. Example: 1970-01-29 until 1971-02-28, largestUnit years Old result: 1 year, 1 month New result: 1 year, 30 days Note that largestUnit weeks and largestUnit days, the latter of which is the default, are not affected. TODO: Make the algorithm in the reference code faster, to help show how it could be implemented. Closes: #2535
This adjusts the difference algorithm for Gregorian-year dates so that when an intermediate date occurs past the end of a month, it is not shifted to the end of that month. Previously, in some edge cases where taking the difference in months or years would return a number of months and zero days, we now return one month less and 28, 29, or 30 days instead. Example: 1970-01-29 until 1971-02-28, largestUnit years Old result: 1 year, 1 month New result: 1 year, 30 days Note that largestUnit weeks and largestUnit days, the latter of which is the default, are not affected. Closes: #2535
|
I never followed up on this issue after the 2024-01-25 and 2024-01-26 champions meetings. We decided to use the "hybrid" algorithm. I refactored the existing algorithm to be structured more like the "hybrid" algorithm, and it turns out that the difference between the two is just one line: namely, whether you constrain the date before determining whether it "surpasses" the target date. This change is made in #2759, which is to be presented at the TC39 plenary next week. |
|
Test262 tests in tc39/test262#4004 |
This adjusts the difference algorithm for Gregorian-year dates so that when an intermediate date occurs past the end of a month, it is not shifted to the end of that month. Previously, in some edge cases where taking the difference in months or years would return a number of months and zero days, we now return one month less and 28, 29, or 30 days instead. Example: 1970-01-29 until 1971-02-28, largestUnit years Old result: 1 year, 1 month New result: 1 year, 30 days Note that largestUnit weeks and largestUnit days, the latter of which is the default, are not affected. Closes: #2535
This adjusts the difference algorithm for Gregorian-year dates so that when an intermediate date occurs past the end of a month, it is not shifted to the end of that month. Previously, in some edge cases where taking the difference in months or years would return a number of months and zero days, we now return one month less and 28, 29, or 30 days instead. Example: 1970-01-29 until 1971-02-28, largestUnit years Old result: 1 year, 1 month New result: 1 year, 30 days Note that largestUnit weeks and largestUnit days, the latter of which is the default, are not affected. Closes: #2535
The current
DifferenceISODatedefinition returns the following results. Notice that it's always"P1M".When the end date is
1970-02-27, the results are more like what I'd expect:Also compare to java.time.temporal, which doesn't always return
"P1M":The text was updated successfully, but these errors were encountered: