Add Dangi to generate_reference_years, fix reference year checking test #7013
Conversation
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. |
The code and data used for fetching this will be pushed up to a separate (private) Unicode repo once we have one. You can find the cleaned up source data in https://gist.github.com/Manishearth/d8c94a7df22a9eacefc4472a5805322e. I'm imagining that post-1950 data will change or be removed with #7006 The initial motivation here was to fix the apparent ground truth mismatch found in https://github.com/unicode-org/icu4x/pull/7007/files#r2393049682. Turns out it was a different problem, and it has been fixed in #7013. We may potentially need the same discussion as #6970 about whether we care about these pre-1912 dates, since that's the only time this diverges.
| (11, false, false) if day > 26 => 1971, | ||
| (11, false, false) => 1972, | ||
| (11, false, true) => 1969, | ||
| (11, false, true) => 1972, |
There was a problem hiding this comment.
@robertbastian This is incorrect: This date is Jan 4, 1973, which is after Dec 31, 1972.
The algorithm needs to look backwards from Dec 31.
There was a problem hiding this comment.
Unfortunately your code doesn't currently handle the Dec 31 boundary so this still needs manual edits.
There was a problem hiding this comment.
ah you had this like this in your PR so I changed the code to produce that (49c9dc6). I thought it needs to be a strict comparison
There was a problem hiding this comment.
Ah I see! Yeah, that works.
Hadn't looked at the next commit.
In https://github.com/unicode-org/icu4x/pull/7007/files#r2393049682 I noticed a Korean reference year was incorrect. At first I thought it was due to a KASI mismatch, but it turns out our algorithm matches KASI from 1912 onwards (#7008), and you can see the bug in our code too.
The bug was due to
generate_reference_years()not doing Dangi: it can be edited to do Dangi, but you have to be careful to update it in two places. If you only changenew_china()tonew_dangi()you get buggy data.I was surprised this wasn't caught by the reference year test. Turns out; the reference year test discards all dates that don't successfully go through MonthDay, and currently if the produced reference year is invalid, MonthDay construction errors.
I updated the test to instead understand what monthday combinations are valid for each calendar.