icu icon indicating copy to clipboard operation
icu copied to clipboard

ICU-20178 Initial checkin of Tibetan Calendar

Open eroux opened this issue 2 years ago • 14 comments

Checklist
  • [X] Required: Issue filed: https://unicode-org.atlassian.net/browse/ICU-20178
  • [X] Required: The PR title must be prefixed with a JIRA Issue number.
  • [X] Required: The PR description must include the link to the Jira Issue, for example by completing the URL in the first checklist item
  • [ ] Required: Each commit message must be prefixed with a JIRA Issue number.
  • [ ] Issue accepted (done by Technical Committee after discussion)
  • [X] Tests included, if applicable
  • [ ] API docs and/or User Guide docs changed or added, if applicable

This is a new PR with updates from https://github.com/unicode-org/icu/pull/1793 . Unfortunately I couldn't contact @dyrroth-11 and I don't have write access to his branch, so here is a new PR, really sorry for that. The PR addresses all the changes requested in #1793 (I think) and adds a little bit of documentation.

Not all tests pass though, and I think I will need a little help to understand why. @FrankYFTang is this something we could discuss? Thanks

eroux avatar Jan 28 '23 15:01 eroux

Thanks a lot for this! It would be super useful to have UCAL_ORDINAL_DAY for the Tibetan (and Hindu and Southeast Asian calendars), do you think it would be possible to add it?

eroux avatar Jan 31 '23 04:01 eroux

Thanks a lot for this! It would be super useful to have UCAL_ORDINAL_DAY for the Tibetan (and Hindu and Southeast Asian calendars), do you think it would be possible to add it?

what do you mean? It is added to the Calendar base class already, you need to implement it in this PR for that. Take a look at the changes to the Chinese calendar which also have leap month to undertsand what it take https://github.com/unicode-org/icu/pull/2274 to implement for Tibetan

FrankYFTang avatar Jan 31 '23 08:01 FrankYFTang

I also see the following error

      CalendarLimitTest {
         TestCalendarExtremeLimit {
         } OK:   TestCalendarExtremeLimit 
         TestLimits {
         FAIL: [tibetan] Maximum value of DAY_OF_WEEK_IN_MONTH is incorrect: 5/expected: 14
intltest: ../../icu4c/source/i18n/calendar.cpp:1670: void icu_73::Calendar::computeWeekFields(UErrorCode&): Assertion `fFields[UCAL_DAY_OF_WEEK_IN_MONTH] <= getMaximum(UCAL_DAY_OF_WEEK_IN_MONTH)' failed.
         FAIL: [tibetan] Least maximum value of DAY_OF_WEEK_IN_MONTH is incorrect: 5/expected: 14```

FrankYFTang avatar Jan 31 '23 08:01 FrankYFTang

I only see UCAL_ORDINAL_MONTH in the base class (here right?), not UCAL_ORDINAL_DAY, or did it miss it? In many Asian calendars, nominal day of the month often go in sequences like 1, 2, 4, 5, 5, 6, etc.

eroux avatar Jan 31 '23 08:01 eroux

I only see UCAL_ORDINAL_MONTH in the base class (here right?), not UCAL_ORDINAL_DAY, or did it miss it? In many Asian calendars, nominal day of the month often go in sequences like 1, 2, 4, 5, 5, 6, etc.

oh sorry my bad. I didn't realize you are saying "DAY" (blame my eyes) here. Is it needed for these two Tibetan calendars? or could you list out all the calendar you are aware about of such? thanks

FrankYFTang avatar Jan 31 '23 21:01 FrankYFTang

No problem!

It is needed for most calendar of South Asian origin, such as the Hindu Lunisolar calendars, the various Tibetan, Bhutanese, Mongolian, Thai, Khmer, Burmese, etc. calendars

eroux avatar Jan 31 '23 22:01 eroux

@FrankYFTang I've pushed some updates implementing your requests. Two things:

  • I've opened a separate issue for UCAL_ORDINAL_DAY: https://unicode-org.atlassian.net/browse/ICU-22275 since it's probably best discussed outside of this PR
  • the failure of the test Maximum value of DAY_OF_WEEK_IN_MONTH is, I have to say, a bit beyond me... I'm not even sure what it tests or what part of the code I'm supposed to look at... what would be your advice?

eroux avatar Feb 11 '23 10:02 eroux

  • the failure of the test Maximum value of DAY_OF_WEEK_IN_MONTH is, I have to say, a bit beyond me... I'm not even sure what it tests or what part of the code I'm supposed to look at... what would be your advice?

I asked around, that name is confusing. It basically, which "7 weeks" in that month. It should not go > 5 since that will mean that month has more than 35 days.

FrankYFTang avatar Feb 27 '23 23:02 FrankYFTang

sorry, I mistakenly click the wrong button. reopen

FrankYFTang avatar Feb 27 '23 23:02 FrankYFTang

hmm ok... it's pretty mysterious since none of the months have more than 30 days and I haven't touched the code for day of the week (they're the same in the Tibetan calendar)...

eroux avatar Feb 28 '23 07:02 eroux

while I understand you need the Calendar API (and likely also DateFormat) to enhance to handle the leap day issue, I belive the current breakage in this PR has nothing to do with that.

FrankYFTang avatar Apr 20 '23 22:04 FrankYFTang

right, I should have some time early May to dive into the failing tests

eroux avatar Apr 21 '23 12:04 eroux