Editorial: Spec out ToISOWeekOfYear w/ algorithm #2378

FrankYFTang · 2022-08-16T02:12:21Z

Also refactor ToISODayOfYear and ToISODayOfWeek.

@ptomato Based on algorithm in https://github.com/tc39/proposal-temporal/blob/main/polyfill/lib/ecmascript.mjs#L2387

Also refactor ToISODayOfYear and ToISODayOfWeek.

codecov · 2022-08-16T02:14:34Z

Codecov Report

Merging #2378 (457be37) into main (5a84124) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2378      +/-   ##
==========================================
- Coverage   95.03%   95.02%   -0.01%     
==========================================
  Files          20       20              
  Lines       10812    10817       +5     
  Branches     1927     1929       +2     
==========================================
+ Hits        10275    10279       +4     
- Misses        503      504       +1     
  Partials       34       34

Impacted Files	Coverage Δ
polyfill/lib/ecmascript.mjs	`98.30% <0.00%> (-0.02%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

FrankYFTang · 2022-08-16T02:14:51Z

address #1641

FrankYFTang · 2022-08-16T17:41:10Z

@sffc @ryzokuken @gibson042

spec/calendar.html

Co-authored-by: Jordan Harband <[email protected]>

FrankYFTang · 2022-08-16T18:28:44Z

@ljharb thanks

ljharb · 2022-08-16T18:30:25Z

spec/calendar.html

+        1. If _dayOfWeek_ is 0, then
+          1. Set _dayOfWeek_ to 7.
+        1. Let _week_ be floor((_dayOfYear_ - _dayOfWeek_ + 10) / 7).
+        1. If _week_ &lt; 1, then
+          1. Let _dayOfJan1st_ be ToISODayOfWeek(_year_, 1, 1).
+          1. If _dayOfJan1st_ is 5, then
+            1. Return 53.
+          1. If _dayOfJan1st_ is 6, then
+            1. Return 52 + ℝ(InLeapYear(TimeFromYear(𝔽(_year_ - 1)))).
+          1. Return 52.
+        1. If _week_ is 53, then
+          1. Let _test_ be InLeapYear(TimeFromYear(𝔽(_year_))) + 365 - _dayOfYear_ - 4 + _dayOfWeek_.
+          1. If _test_ &lt; 0, then
+            1. Return 1.


these are a lot of magic numbers - can they be placed in named constants?

I got them from the polyfill
It is truly a magic number to me. It will be nice if the polyfill author could suggest the name for such constant.

@redneb @ptomato @pipobscure

See my comment at #1641 (comment)

ljharb · 2022-08-16T18:31:28Z

spec/calendar.html

+        1. Let _epochDays_ be MakeDay(year, month - 1, day).
+        1. Assert: _epochDays_ is finite.
+        1. Let _dayOfWeek_ be WeekDay(MakeDate(_epochDays_, *+0*<sub>𝔽</sub>)).
+        1. If _dayOfWeek_ = *+0*<sub>𝔽</sub>, return *7*<sub>𝔽</sub>.


can you elaborate on this line? is sunday the last day of the week or the first in ISO?

Monday is 1, Sunday is 7.

pipobscure · 2022-08-16T21:02:59Z

enum WeekDay {
Monday=1;
Tuesday;
Wednesday;
Thursday;
Friday;
Saturday;
Sunday;
}

however the utility function ES.DayOfWeek follows the Date.prototype.getDay convention which put Sunday as 0 for Weekday-Array-Indexing-in-the-US reasons.

Temporals dayOfWeek uses the 1..7 ISO values.

So step-1 is superfluous.

The 10 in step-2 comes from the rule being that week-1 is the week containing the first Thursday of the year. (7days in a full week + 3days before a Thursday).

The rest of the constants are just from the Day-enum or actual week-numbers.

FrankYFTang · 2022-08-16T21:41:35Z

Add const, PTAL

ljharb · 2022-08-16T21:41:18Z

spec/calendar.html

+        1. If _week_ &lt; 1, then
+          1. Let _dayOfJan1st_ be ToISODayOfWeek(_year_, 1, 1).
+          1. If _dayOfJan1st_ is _friday_, then
+            1. Return 53.


could we have some kind of "weeks in a year" constant for 52, and this can be that plus 1?

ljharb · 2022-08-16T21:42:03Z

spec/calendar.html

+        1. If _dayOfWeek_ is 0, then
+          1. Set _dayOfWeek_ to _saturday_.


this seems confusing to me; it would be clearer to explicitly map the entire 0-6 range to 1-7 mod 7 (also isn't this one supposed to be sunday, not saturday?)

pipobscure · 2022-08-16T21:56:14Z

Sorry, the latest changes are very wrong, so I have not been nearly as clear as I hoped. Alas I’m on my phone right now and therefor typing impaired.

Can I come back to you both tomorrow with a clearer algorithm spec.

pipobscure · 2022-08-17T08:33:13Z

Week of Year Algorithm

Assert: year is an integer
Assert: month is an integer between 1 and 12 inclusive
If month is 1, 3, 5, 7, 8, 10 or 12
1. Assert: day is an integer between 1 and 31 inclusive
If month is 4,6,9, or 11
1. Assert: day is an integer between 1 and 30 inclusive
If month is 2
1. If ES.IsLeapYear(year)
  1. Assert: day is an integer between 1 and 29 inclusive
2. Otherwise
  1. Assert: day is an integer between 1 and 28 inclusive
Let dayOfYear be ToISODayOfYear(year , month , day)
Let dayOfWeek be ToISODayOfWeek(year , month , day)
Let dayOfWeekJan1 be ToISODayOfWeek(year , 1 , 1)
Let daysInWeek be 7
Let wednesday be 3
Let thursday be 4
Let week be floor((dayOfYear - (daysInWeek - dayOfWeek + wednesday)) / daysInWeek)
If week < 1 <!— Last week of previous year —>
1. Let friday be 5
2. Let saturday be 6
3. If dayOfWeekJan1 is friday
  1. Return 53
4. If _ dayOfWeekJan1_ is saturday and ES.IsLeapYear(year - 1)
  1. Return 53
5. Return 52
If week is 53
1. If ES.IsLeapYear(year)
  1. Let daysInYear be 366
2. Otherwise
  1. Let daysInYear be 365
3. Let daysLaterInYear be daysInYear - dayOfYear
4. Let daysAfterThursday be thursday - dayOfWeek
5. If daysLaterInYear < daysAfterThursday
  1. Return 1
Return week

ljharb · 2022-08-17T15:03:24Z

Steps 3-5 there seem like they’d make a very helpful separate AO to encapsulate that validation.

It’s still a bit confusing why specific days (January 5th, 6th, etc) are magic values.

ptomato · 2022-08-17T16:49:54Z

For steps 3–5 we have IsValidISODate

pipobscure · 2022-08-17T17:11:20Z

It’s still a bit confusing why specific days (January 5th, 6th, etc) are magic values.

The formula in step 12 has as a consequence that the week is only smaller than 1 if January 1st is either Friday, Saturday or Sunday. In any of those cases the week number is the last one of the previous year. So it’s just a question of which one that is.

(Note: I also corrected the algorithm because it had an error in that section by referring to jan5 & jan6)

FrankYFTang · 2022-08-17T17:36:40Z

sorry, I accidentally click the wrong key

gibson042 · 2022-08-17T20:39:55Z

It would be a good idea to explain with reference to ISO 8601-1:2019 section 3.1.1.23 week calendar:

the first calendar week of a calendar year is the week including the first Thursday of that year
Note 1 to entry: This rule is based on the principle that a week belongs to the calendar year to which the majority of its calendar days (3.1.2.11) belong.
Note 2 to entry: In the week calendar, calendar days of the first and last calendar week of a calendar year may belong to the previous and the next calendar year respectively

Note that this supports rather comprehensible algorithms—ISO week 01 of every calendar year includes its January 4 and starts on the first Monday on or before that, the ISO week calendar year of any date is its calendar year adjusted by -1 if it precedes that Monday or by +1 if it falls on or after that Monday of week 01 for the next calendar year, and the ISO week number of any date is 1 plus the number of complete weeks from the start of its week calendar week 01 (e.g., 1 + floor((Day(t) - Day(weekCalendarStart)) / 7)).

Or alternatively, the algorithms can use lookups against compiled data such as the table at https://en.wikipedia.org/wiki/ISO_week_date#First_week .

FrankYFTang · 2022-08-18T23:39:58Z

fix linter error, PTAL

spec/calendar.html

gibson042 · 2022-08-19T14:58:04Z

spec/calendar.html

+        1. Let _week_ be floor((_dayOfYear_ - (_daysInWeek_ - _dayOfWeek_ + _wednesday_ )) / _daysInWeek_).
+        1. If _week_ &lt; 1, then
+          1. NOTE: This is the last week of the previous year.


This formula seems to be incorrect. Consider 2023-01-02, which is the Monday at the start of 2023-W01: https://www.timeanddate.com/calendar/monthly.html?year=2023&month=1&country=9

Let dayOfYear be ToISODayOfYear(year, month, day). → 2

Let dayOfWeek be ToISODayOfWeek(year, month, day). → 1

Let week be floor((dayOfYear - (daysInWeek - dayOfWeek + wednesday )) / daysInWeek) → floor((2 - (7 - 1 + 3 )) / 7) → floor(-1) → -1

If week < 1, then

NOTE: This is the last week of the previous year.

Which is an inaccurate classification of the input date.

spec/calendar.html

Co-authored-by: Richard Gibson <[email protected]>

FrankYFTang · 2022-08-22T17:34:10Z

Let week be floor((dayOfYear - (daysInWeek - dayOfWeek + wednesday)) / daysInWeek)

Should this be

Let week be floor((dayOfYear + (daysInWeek - dayOfWeek + wednesday)) / daysInWeek)
?

FrankYFTang · 2022-08-22T17:37:26Z

Polyfill code is

const week = MathFloor((doy - dow + 10) / 7);
So... it should be
Let week be floor((dayOfYear + daysInWeek - dayOfWeek + wednesday ) / daysInWeek).

instead

FrankYFTang · 2022-08-22T17:38:20Z

2023-01-02 then will be
Let week be floor((dayOfYear + (daysInWeek - dayOfWeek + wednesday )) / daysInWeek) → floor((2 + (7 - 1 + 3 )) / 7) → floor(1) → 1

gibson042

The algorithm is a bit opaque as compared to something based on #2378 (comment) such as

1. Let _weekCalendarYear_ be _year_.
1. Let _jan4DayOfWeek_ be ToISODayOfWeek(_weekCalendarYear_, 1, 4).
1. Let _week01StartDayOfYear_ be 4 - _jan4DayOfWeek_ + 1.
1. NOTE: If _week01StartDayOfYear_ is less than 1, then the week calendar year starts in the last few calendar days of calendar year _year_ - 1.
1. Let _dayOfYear_ be ToISODayOfYear(_year_, _month_, _day_).
1. If _dayOfYear_ &ge; 363, then
  1. Let _dayOfWeek_ be ToISODayOfWeek(_year_, _month_, _day_).
  1. Let _daysUntilThisWeekThursday_ be 4 - _dayOfWeek_.
  1. If _daysUntilThisWeekThursday_ &gt; 0 and (_dayOfYear_ + _daysUntilThisWeekThursday_) &gt; DaysInYear(𝔽(_weekCalendarYear_)), then
    1. NOTE: The upcoming Thursday is in the next calendar year, so the entire containing week is the first week of that week calendar year.
    1. Return 1.
1. If _dayOfYear_ &lt; _week01StartDayOfYear_, then
  1. NOTE: The date belongs to the last week of the previous week calendar year.
  1. Set _weekCalendarYear_ to _year_ - 1.
  1. Set _jan4DayOfWeek_ to ToISODayOfWeek(_weekCalendarYear_, 1, 4).
  1. Set _week01StartDayOfYear_ to 4 - _jan4DayOfWeek_ + 1.
  1. Set _dayOfYear_ to DaysInYear(𝔽(_weekCalendarYear_)) + _dayOfYear_.
1. Assert: _dayOfYear_ &ge; _week01StartDayOfYear_.
1. Let _daysSinceStart_ be _dayOfYear_ - _week01StartDayOfYear_.
1. Return 1 + floor(_daysSinceStart_ / 7).

Regardless, it seems correct to me.

FrankYFTang · 2022-08-22T23:52:10Z

Can we got all approved and merge this PR ASAP? Thanks

FrankYFTang · 2022-08-23T16:52:49Z

@ljharb any other comments?

FrankYFTang · 2022-08-23T16:53:56Z

The algorithm is a bit opaque as compared to something based on #2378 (comment) such as

That is because I changed the PR to follow #2378 (comment)

ljharb · 2022-08-23T16:57:06Z

I still find the algorithm a bit inscrutable due to the use of arbitrary days of the week, but it seems correct enough.

FrankYFTang · 2022-08-23T17:21:32Z

the v8 implementation sync to this could be found in https://chromium-review.googlesource.com/c/v8/v8/+/3531567 Adam expressed he prefer we finalize the algorithm in spec text before he continue the review so this PR is blocking the merge of that.

ptomato · 2022-08-23T18:04:53Z

I'm OK with merging this as it's blocking the V8 implementation. (However skeptical I am that it lends any extra correctness to the V8 implementation to have this finalized first, since we are just pulling in an algorithm from somewhere else too.)

I didn't double-check the most recent change from subtraction to addition in the algorithm, but I trust that @gibson042 did.

Thanks, @FrankYFTang for putting this together.

Also add AO: ToISOWeekOfYear Spec Text: https://tc39.es/proposal-temporal/#sec-temporal.calendar.prototype.weekofyear https://tc39.es/proposal-temporal/#sec-temporal-toisoweekofyear Note- this is only the non-intl version. intl version in https://tc39.es/proposal-temporal/#sup-temporal.calendar.prototype.weekofyear will be implemented in later cl. PR tc39/proposal-temporal#2378 Sync spec text for ToISODayOfYear and ToISODayOfWeek in the comment and add DCHECK for assertion. Bug: v8:11544 Change-Id: If07ff76551707d17d125e41bc624c12da6efa45a Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3531567 Commit-Queue: Frank Tang <[email protected]> Reviewed-by: Shu-yu Guo <[email protected]> Cr-Commit-Position: refs/heads/main@{#82733}

Editorial: Spec out ToISOWeekOfYear w/ algorithm

ed22add

Also refactor ToISODayOfYear and ToISODayOfWeek.

fix linter

1fb318f

ljharb reviewed Aug 16, 2022

View reviewed changes

spec/calendar.html Outdated Show resolved Hide resolved

spec/calendar.html Outdated Show resolved Hide resolved

spec/calendar.html Outdated Show resolved Hide resolved

FrankYFTang and others added 3 commits August 16, 2022 11:27

Update spec/calendar.html

0b1087e

Co-authored-by: Jordan Harband <[email protected]>

Update spec/calendar.html

8eff723

Co-authored-by: Jordan Harband <[email protected]>

Update spec/calendar.html

561572d

Co-authored-by: Jordan Harband <[email protected]>

FrankYFTang requested a review from ljharb August 16, 2022 18:29

ljharb reviewed Aug 16, 2022

View reviewed changes

FrankYFTang added 2 commits August 16, 2022 14:38

add Const

b72f3f3

correct to sunday

c011c82

ljharb reviewed Aug 16, 2022

View reviewed changes

Add more const

daa3fdd

FrankYFTang added 2 commits August 17, 2022 10:30

update by taking some of pipobscure's idea

b89b7b8

rename constant

47478c0

FrankYFTang closed this Aug 17, 2022

FrankYFTang reopened this Aug 17, 2022

FrankYFTang added 2 commits August 18, 2022 16:36

fix lint

98474ce

fix lint

de5dbc7

FrankYFTang requested review from ptomato and ljharb and removed request for ptomato and ljharb August 18, 2022 23:39

gibson042 requested changes Aug 19, 2022

View reviewed changes

FrankYFTang and others added 4 commits August 22, 2022 10:27

Update spec/calendar.html

9d5c032

Co-authored-by: Richard Gibson <[email protected]>

Update spec/calendar.html

e80b2cd

Co-authored-by: Richard Gibson <[email protected]>

Update spec/calendar.html

78a0222

Co-authored-by: Richard Gibson <[email protected]>

Update spec/calendar.html

276c173

Co-authored-by: Richard Gibson <[email protected]>

fix formula

457be37

gibson042 approved these changes Aug 22, 2022

View reviewed changes

ptomato merged commit 33b62a3 into tc39:main Aug 23, 2022

ptomato mentioned this pull request Aug 23, 2022

Need to be more specific about "according to ISO-8601" in ToISOWeekOfYear #1641

Closed

ptomato mentioned this pull request Sep 15, 2022

Reconsider year-week support in light of implementation feedback #2405

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Editorial: Spec out ToISOWeekOfYear w/ algorithm #2378

Editorial: Spec out ToISOWeekOfYear w/ algorithm #2378

FrankYFTang commented Aug 16, 2022

codecov bot commented Aug 16, 2022 •

edited

Loading

FrankYFTang commented Aug 16, 2022

FrankYFTang commented Aug 16, 2022

FrankYFTang commented Aug 16, 2022

ljharb Aug 16, 2022

FrankYFTang Aug 16, 2022 •

edited

Loading

ptomato Aug 16, 2022

ljharb Aug 16, 2022

ptomato Aug 18, 2022

pipobscure commented Aug 16, 2022 •

edited

Loading

FrankYFTang commented Aug 16, 2022

ljharb Aug 16, 2022

ljharb Aug 16, 2022

pipobscure commented Aug 16, 2022

pipobscure commented Aug 17, 2022 •

edited

Loading

ljharb commented Aug 17, 2022

ptomato commented Aug 17, 2022

pipobscure commented Aug 17, 2022 •

edited

Loading

FrankYFTang commented Aug 17, 2022

gibson042 commented Aug 17, 2022

FrankYFTang commented Aug 18, 2022

gibson042 Aug 19, 2022

FrankYFTang commented Aug 22, 2022

FrankYFTang commented Aug 22, 2022

FrankYFTang commented Aug 22, 2022 •

edited

Loading

gibson042 left a comment •

edited

Loading

FrankYFTang commented Aug 22, 2022

FrankYFTang commented Aug 23, 2022

FrankYFTang commented Aug 23, 2022

ljharb commented Aug 23, 2022

FrankYFTang commented Aug 23, 2022

ptomato commented Aug 23, 2022

		1. If _dayOfWeek_ is 0, then
		1. Set _dayOfWeek_ to _saturday_.

Editorial: Spec out ToISOWeekOfYear w/ algorithm #2378

Editorial: Spec out ToISOWeekOfYear w/ algorithm #2378

Conversation

FrankYFTang commented Aug 16, 2022

codecov bot commented Aug 16, 2022 • edited Loading

Codecov Report

FrankYFTang commented Aug 16, 2022

FrankYFTang commented Aug 16, 2022

FrankYFTang commented Aug 16, 2022

ljharb Aug 16, 2022

Choose a reason for hiding this comment

FrankYFTang Aug 16, 2022 • edited Loading

Choose a reason for hiding this comment

ptomato Aug 16, 2022

Choose a reason for hiding this comment

ljharb Aug 16, 2022

Choose a reason for hiding this comment

ptomato Aug 18, 2022

Choose a reason for hiding this comment

pipobscure commented Aug 16, 2022 • edited Loading

FrankYFTang commented Aug 16, 2022

ljharb Aug 16, 2022

Choose a reason for hiding this comment

ljharb Aug 16, 2022

Choose a reason for hiding this comment

pipobscure commented Aug 16, 2022

pipobscure commented Aug 17, 2022 • edited Loading

Week of Year Algorithm

ljharb commented Aug 17, 2022

ptomato commented Aug 17, 2022

pipobscure commented Aug 17, 2022 • edited Loading

FrankYFTang commented Aug 17, 2022

gibson042 commented Aug 17, 2022

FrankYFTang commented Aug 18, 2022

gibson042 Aug 19, 2022

Choose a reason for hiding this comment

FrankYFTang commented Aug 22, 2022

FrankYFTang commented Aug 22, 2022

FrankYFTang commented Aug 22, 2022 • edited Loading

gibson042 left a comment • edited Loading

Choose a reason for hiding this comment

FrankYFTang commented Aug 22, 2022

FrankYFTang commented Aug 23, 2022

FrankYFTang commented Aug 23, 2022

ljharb commented Aug 23, 2022

FrankYFTang commented Aug 23, 2022

ptomato commented Aug 23, 2022

codecov bot commented Aug 16, 2022 •

edited

Loading

FrankYFTang Aug 16, 2022 •

edited

Loading

pipobscure commented Aug 16, 2022 •

edited

Loading

pipobscure commented Aug 17, 2022 •

edited

Loading

pipobscure commented Aug 17, 2022 •

edited

Loading

FrankYFTang commented Aug 22, 2022 •

edited

Loading

gibson042 left a comment •

edited

Loading