icu icon indicating copy to clipboard operation
icu copied to clipboard
verified publisher icon unicode-org

ICU-11533 Myanmar calendar support - suggestion to add [WIP]

Open mapmeld opened this issue 6 years ago • 18 comments

This is still a work in progress, but as an ICU and C++ newbie, I am reaching out for advice. Any help on setup, please contact me here or @mapmeld on Twitter.

Update: earlier I was overwriting BuddhCal.cpp with MyanCal.cpp to get it to run; for the PR to be more legible I have changed them back to separate files

Checklist
  • [x] Issue filed: https://unicode-org.atlassian.net/browse/ICU-11533
  • [x] Updated PR title and link in previous line to include Issue number
  • [x] Issue accepted
  • [x] Tests included
  • [ ] Documentation is changed or added

CLDR ticket: https://unicode-org.atlassian.net/browse/CLDR-8081 Unicode / ICU ticket: https://unicode-org.atlassian.net/browse/ICU-11533

The code implements the Burmese/Myanmar lunisolar calendar for festival / holiday dates. Some people in Myanmar use this calendar on official documents and forms, e.g. their birthdate, and it is not simple to convert between them.

Comparing to a working reference

I am using the open source C++ and JS implementations from https://github.com/yan9a/mcal and making changes:

  • renaming variables so their meaning is easier to understand
  • changing variable type from long to int where a long is not needed (e.g. day / month / year )
  • numbering months in chronological order
  • using ClockMath.floorDivide instead of importing math.h ... but is this necessary? Following other calendar implementations Currently the tests appear to have off-by-one errors (1 month and 1 day off) in most cases.

Help with development

~~As mentioned earlier, development would be better if I can create calendar=myanmar separate from calendar=buddhist. Also~~ I currently run all icu4c tests with make check instead of the ones which I am targeting - Any help on setup, please contact me here or @mapmeld on Twitter.

CLA

I have signed the CLA. Because the C++ code is based on Yan Naing Aye's repo, I've filed an Issue asking if he would sign the CLA as well.

mapmeld avatar Apr 22 '19 00:04 mapmeld

I changed the PR so it is more legible - I no longer overwrite BuddhistCalendar

Update: the below issue was resolved by updating enumeration in calendar.cpp

~~Here are the errors which I see preventing creation of a new calendar type (these errors do not raise when I overwrote an existing calendar)~~

 UObjectTest {
         testIDs {
         FAIL: dynamicID != staticID! MyanmarCalendar * x= Calendar::createInstance(Locale("@calendar=myanmar"), status);  - (Are you missing data?)

         } ERRORS (1) in testIDs (16ms)

mapmeld avatar Apr 22 '19 02:04 mapmeld

Notice: the branch changed across the force-push!

  • icu4c/source/i18n/myancal.h is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

jira-pull-request-webhook[bot] avatar Apr 22 '19 02:04 jira-pull-request-webhook[bot]

@mapmeld thanks for this! The automated builds are failing (look for the red X's above), see https://travis-ci.org/unicode-org/icu/jobs/522911134#L1351 for example.

srl295 avatar Apr 24 '19 22:04 srl295

@srl295 progress update / question: am I allowed to import math.h here? The other calendars use ClockMath::floorDivide instead, but it is giving me an unusual (negative) result on one line dividing doubles, causing everything to be shifted one lunar month later on in the code

ClockMath::floorDivide(double(SOLAR_YEAR * (long(myan_year) + 3739)), LUNAR_MONTH, excess_days);

mapmeld avatar May 08 '19 19:05 mapmeld

@mapmeld math.h is probably OK, as astro.cpp and others use it. If you can use the uprv_ functions such as uprv_floor() that gives a little more platform specific separation.

Note that floorDivide is implemented via uprv_floor(a/b) which just calls floor.

Specifically, though, use of long is better changed to int32_t so that it is a fixed width.

srl295 avatar May 08 '19 22:05 srl295

Notice: the branch changed across the force-push!

  • .gitignore is no longer changed in the branch
  • icu4c/source/data/lang/en.txt is no longer changed in the branch
  • icu4c/source/data/locales/root.txt is different
  • icu4c/source/data/misc/keyTypeData.txt is no longer changed in the branch
  • icu4c/source/data/misc/supplementalData.txt is different
  • icu4c/source/i18n/myancal.h is different
  • icu4c/source/i18n/ucal.cpp is different
  • icu4c/source/test/intltest/callimts.cpp is different
  • icu4c/source/test/intltest/erarulestest.cpp is different
  • icu4c/source/test/intltest/uobjtest.cpp is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

jira-pull-request-webhook[bot] avatar May 09 '19 01:05 jira-pull-request-webhook[bot]

@mapmeld ping on this?

srl295 avatar Mar 24 '20 22:03 srl295

I got formal approval from the original author of the C++ code. Unfortunately I haven't looked at this for a while, and never found a good way to debug what was happening. I feel like this is VERY close but only for someone who understands C++ and can find out why it is usually off by one lunar month.

mapmeld avatar Mar 25 '20 19:03 mapmeld

I got formal approval from the original author of the C++ code. Unfortunately I haven't looked at this for a while, and never found a good way to debug what was happening. I feel like this is VERY close but only for someone who understands C++ and can find out why it is usually off by one lunar month.

didn't get to debug it but the 'month' field does start at 0 of course, that always gets me… Can you rebase it against current master?

Also, if you didn't already do so, we'll definitely need a CLDR ticket for this, and the initial english/etc names should go there. IF you start a PR that adds the right stuff into CLDR's en.xml, etc, i can help with the data generation

srl295 avatar Mar 25 '20 20:03 srl295

Notice: the branch changed across the force-push!

  • icu4c/source/data/locales/root.txt is no longer changed in the branch
  • icu4c/source/data/misc/supplementalData.txt is no longer changed in the branch
  • icu4c/source/i18n/calendar.cpp is different
  • icu4c/source/i18n/i18n_uwp.vcxproj is different
  • icu4c/source/i18n/i18n.vcxproj is different
  • icu4c/source/i18n/i18n.vcxproj.filters is different
  • icu4c/source/i18n/Makefile.in is no longer changed in the branch
  • icu4c/source/i18n/myancal.cpp is different
  • icu4c/source/i18n/myancal.h is different
  • icu4c/source/i18n/ucal.cpp is different
  • icu4c/source/test/depstest/dependencies.txt is different
  • icu4c/source/test/intltest/callimts.cpp is different
  • icu4c/source/test/intltest/erarulestest.cpp is no longer changed in the branch
  • icu4c/source/test/intltest/incaltst.cpp is different
  • icu4c/source/test/intltest/incaltst.h is no longer changed in the branch
  • icu4c/source/test/intltest/uobjtest.cpp is different
  • icu4c/source/test/testdata/structLocale.txt is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

jira-pull-request-webhook[bot] avatar Jun 05 '20 21:06 jira-pull-request-webhook[bot]

@srl295 sorry for another long delay on responding to the project. I've updated to the current master branch and reapplied my changes in one commit. I have tests for MyanCal but they have been failing, off by one lunar month. I don't know how to test my code without the entire ICU test suite, which makes it difficult for me to switch back and forth between coding and testing or printf-ing to figure out where I'm losing the correct values.

mapmeld avatar Jun 05 '20 21:06 mapmeld

@srl295 sorry for another long delay on responding to the project. I've updated to the current master branch and reapplied my changes in one commit. I have tests for MyanCal but they have been failing, off by one lunar month. I don't know how to test my code without the entire ICU test suite, which makes it difficult for me to switch back and forth between coding and testing or printf-ing to figure out where I'm losing the correct values.

This is what I do on Linux:

  • git repo workspace in ~/icu/mine/src
  • out of source build into ~/icu/mine/dbg/icu4c
  • cd ~/icu/mine/dbg/icu4c/test/intltest
  • export LD_LIBRARY_PATH=../../lib:../../stubdata:../../tools/ctestfw
  • make && ./intltest format/IntlCalendarTest

markusicu avatar Nov 07 '22 19:11 markusicu

Notice: the branch changed across the force-push!

  • icu4c/source/i18n/calendar.cpp is different
  • icu4c/source/i18n/i18n_uwp.vcxproj is different
  • icu4c/source/i18n/i18n.vcxproj is different
  • icu4c/source/i18n/i18n.vcxproj.filters is different
  • icu4c/source/i18n/ucal.cpp is different
  • icu4c/source/test/depstest/dependencies.txt is different
  • icu4c/source/test/intltest/callimts.cpp is different
  • icu4c/source/test/intltest/incaltst.cpp is different
  • icu4c/source/test/intltest/uobjtest.cpp is different
  • icu4c/source/test/testdata/structLocale.txt is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

jira-pull-request-webhook[bot] avatar Feb 10 '23 22:02 jira-pull-request-webhook[bot]

I rebased to make this current, in hopes that someone (possibly me, but happy to pass this on) can debug the tests

mapmeld avatar Feb 10 '23 22:02 mapmeld

Notice: the branch changed across the force-push!

  • icu4c/source/test/intltest/incaltst.h is now changed in the branch

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

jira-pull-request-webhook[bot] avatar Feb 11 '23 00:02 jira-pull-request-webhook[bot]