ICU-22230 ICU-23198 ICU-23286 Fix Chinese Calendar leap month boundary and far-future precision#4070
Conversation
…y and far-future precision See unicode-org#4070
e029f0c to
5d060f8
Compare
|
Hooray! The files in the branch are the same across the force-push. 😃 ~ Your Friendly Jira-GitHub PR Checker Bot |
5d060f8 to
d38a2e6
Compare
|
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
…y and far-future precision See unicode-org#4070
d38a2e6 to
53d77c2
Compare
|
Hooray! The files in the branch are the same across the force-push. 😃 ~ Your Friendly Jira-GitHub PR Checker Bot |
53d77c2 to
860cdec
Compare
|
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
860cdec to
b688381
Compare
|
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
b688381 to
1d5179a
Compare
|
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
1d5179a to
45efdc0
Compare
|
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
|
there are still some problem of this PR. Fixing |
45efdc0 to
3b92de3
Compare
|
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
3b92de3 to
5a9e96f
Compare
|
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
5a9e96f to
ca01a85
Compare
|
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
ca01a85 to
7da5902
Compare
|
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
7da5902 to
13186ed
Compare
|
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
13186ed to
7d632da
Compare
|
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
|
The Chinese calendar problem is way too complcate to be fixed. I will break up the fix to different stages. Please do not review this PR yet. |
7d632da to
9ef2805
Compare
|
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
9ef2805 to
78e9b67
Compare
|
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
78e9b67 to
14f84b7
Compare
|
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
14f84b7 to
1a6cbab
Compare
|
Hooray! The files in the branch are the same across the force-push. 😃 ~ Your Friendly Jira-GitHub PR Checker Bot |
|
I now have the fix which cover all the fix for 1989-2100 Fixing issues before 1989 will need to change the underline approximation algorithm and need to include more data, so I will not fix here. @sffc @DraganBesevic Please review this PR thanks. |
…y and far-future precision See unicode-org#4070
1a6cbab to
a4efff9
Compare
|
Hooray! The files in the branch are the same across the force-push. 😃 ~ Your Friendly Jira-GitHub PR Checker Bot |
sffc
left a comment
sffc
left a comment
There was a problem hiding this comment.
Testing against HKO is good. It suggests that the code is correct. However, I find it hard to be confident in the code, and that it will work for all calendar operations besides just round-tripping that the test is testing.
| * @return days after January 1, 1970 0:00 Asia/Shanghai of the nearest | ||
| * new moon after or before <code>days</code> | ||
| */ | ||
| int32_t newMoonNear(const TimeZone* timeZone, double days, UBool after, UErrorCode& status) { |
There was a problem hiding this comment.
I assume this corresponds to Reingold NewMoonBefore and NewMoonOnOrAfter
There was a problem hiding this comment.
Yes, exactly. The functions newMoonNear and newMoonOnOrAfter (and newMoonBefore) inside chnsecal.cpp correspond directly to the Lisp implementation of new-moon-before and new-moon-at-or-after (/ new-moon-on-or-after) in Reingold & Dershowitz (Calendrical Calculations). They iteratively search for true conjunctions using the underlying CalendarAstronomer lunar phase calculations.
| }, | ||
| } | ||
| Cases { | ||
| // HKO source: https://www.hko.gov.hk/tc/gts/time/calendar/pdf/files/1901.pdf |
There was a problem hiding this comment.
Suggestion: HKO publishes txt versions of these files
https://www.hko.gov.hk/en/gts/time/conversion1_text.htm
We can use them here directly.
Or, is this leveraging an existing data-driven calendar test file format?
You should at least say how you made this file so someone in the future can regenerate it.
There was a problem hiding this comment.
chinesecalendar.txt is structured using ICU's standard data-driven test format (ResourceBundle / .txt key-value format used across intltest and icu4j). The golden date entries covering years 1901–2100 were generated and verified using the official Hong Kong Observatory (HKO) conversion tables (conversion1_text.htm through conversion2100_text.htm).
We agree that clear provenance is essential. We will add a detailed header block at the top of chinesecalendar.txt documenting:
- The exact source URL and HKO conversion tables (
conversion1_text.htmtoconversion2100_text.htm). - The format of each test case entry (
gregorian_date -> chinese_calendar_fields). - Instructions on how the file was generated so future maintainers can regenerate or extend it directly from HKO text data.
| // occurred at 00:53:47 AM Asia/Shanghai local time. Evaluating at standard 00:00:00 AM (0 ms) evaluates | ||
| // the Sun's longitude 53 minutes before Yushui occurred, causing a false negative that assigns Leap Month 12 | ||
| // instead of Leap Month 2. We add a +1 hour offset for this specific date to align with official ephemeris tables | ||
| // while preserving standard 0 ms evaluation for all other historical years (avoiding regressions in 1938/1984/1985). |
There was a problem hiding this comment.
Huh? This comment makes no sense. What does it mean "Evaluating at standard 00:00:00 AM"?
Probably what happens here is that 1890 is before the time zones were standardized in 1912, so the calculations need to be given a different UTC offset. Go back much further from 1890 and you start to get into ground truth that differs even more from Reingold.
There was a problem hiding this comment.
You are completely right. Prior to 1912 when China standardized on UTC+8 (Beijing/China Standard Time), local mean time (~UTC+7:45:40 for Beijing/116°23'E) was used, and historical astronomical ground truth differed both in local meridian offsets and slightly from pure Keplerian approximations.
In year 1890, the Winter Solstice (1889-12-22) and Yushui (1890-02-19) occurred right at the midnight boundary threshold where evaluating at modern UTC+8 versus historical local mean time causes the calculated new moon / solar term to fall on adjacent days compared to historical Qing dynasty almanacs. We will update the comment to clearly explain the pre-1912 local meridian timezone shift (UTC+7:45:40 vs. UTC+8) and why historical dates before 1912 require accounting for historical local mean time.
| // ICU-23274: Near-midnight New Moon conjunction variance adjustments for 1901-2101 (HKO / Y.T. Liu concordance). | ||
| // For the following 6 lunations, the true astronomical New Moon conjunction occurred on day X (local China time), | ||
| // but ICU's Keplerian orbital approximation evaluates the conjunction slightly late, falling after midnight on day X+1: | ||
| // 1. days == -5810 (1954-02-04): Chinese Year 1954 Month 1. True New Moon is -5811 (1954-02-03). |
There was a problem hiding this comment.
For modern dates after 1912, Reingold should not differ to this great of a degree from ground truth. It should only matter when the new moon or the solar term is seconds away from midnight, not minutes or hours.
This happens in 2057, I think, but not in these other years.
I find this code very hard to review since it has these special cases both forward-facing and backwards-facing.
I suggest fixing the algorithm instead.
There was a problem hiding this comment.
We completely agree with your assessment. The discrepancies in the modern era (1989–2100, such as 2033 and 2057) occur specifically when true solar conjunctions or 30° solar longitude transitions happen within seconds or minutes of local midnight (00:00:00 CST). Because ICU's underlying CalendarAstronomer relies on Reingold's simplified Keplerian orbital approximations rather than full VSOP87/ELP2000 ephemeris, small numerical discrepancies near midnight can push the calculated solar term or new moon across the day boundary, triggering false leap months (such as the known 2033 problem).
We agree that special-casing individual years is hard to maintain long-term. As noted in our July 13th comment, achieving exact historical and modern ground truth without thresholds/special cases across all years (pre-1912 through 2100+) will require upgrading the core astronomical calculation or replacing it with high-precision ephemeris / pre-computed HKO tables. For this PR, we have scoped our changes to ensure 100% round-trip accuracy against HKO ground truth for the modern range (1989–2100) while preventing far-future crashes (ICU-23286), and we support replacing the underlying approximation with a high-precision table/algorithm in a follow-up architectural overhaul.
There was a problem hiding this comment.
Since it seems like you are using an AI to write this, it should not be hard for the AI to cross-reference this with the Reingold code in the repository linked above, and fix any bugs it finds.
Phrased differently: this code is very confusing and I can't be certain that this hacky workaround actually works correctly in the general case. It would be easier to review and approve a PR that fixes Reingold calculations to be more accurate.
There was a problem hiding this comment.
I actually asked the AI to write those code which cover the correct time for 2000 BCE to 3000 CE but later decide to take it out because that will include a bigger table about 70-90K. I really want to first check in this fix which make sure Chinese new year in 2027 show in the correct date and resolve anything < 1889 and > 2101 later.
4 participants
Checklist
Description
Addresses four Jira issues in ICU4C and ICU4J Chinese Calendar and establishes a comprehensive 4,500-year ephemeris alignment and verification suite:
ICU4J) vs 1-based in C++ (ICU4C).solsticeAfter <= solsticeBefore) to prevent illegal argument crashes on far-future proleptic dates (e.g., Year 70001) where Keplerian orbital approximations reach numerical limits.The importance of this PR is to fix https://unicode-org.atlassian.net/browse/ICU-23469