elixir-cldr / cldr_calendars_lunisolar

Chinese localised calendar for Elixir and CLDR
Other
4 stars 2 forks source link

Result of Lunar to Solar Calendar in Korean calendar is different from actual date. #2

Closed nineclue closed 1 year ago

nineclue commented 1 year ago

Following code converts this year's lunar new year's day correctly.

Date.convert(Date.new!(4356, 1, 1, Cldr.Calendar.Korean), Calendar.ISO) ~D[2023-01-22]

Buddha's birthday (lunar 04/08) is a national holiday in Korea. Result of Date.convert(Date.new(4356, 4, 8, Korean), ISO) is ~D[2023-04-27]. Actual holiday is ~D[2023-05-27] because year 2023 is leap year and leap month is 2.

kipcole9 commented 1 year ago

Thanks for the report and sorry for the bug. This was definitely the trickiest calendar to implement. I'll do my best to get to this over the weekend but resolution may slip into next week.

nineclue commented 1 year ago

Thanks for your kind and quick response.

I can't figure out how you manage to convert lunar date to solar but I'm attaching a zip file which contains numerous inserts clauses that matches lunar date to solar one from 1900/1/1 to 2200/12/31. field 'yun' matches leap month. Hope this data be helpful.

lunartosolar_20020204.zip

kipcole9 commented 1 year ago

@nineclue Took me a while to get my head back in the lunisolar mental model and I realised that what you are reporting isn't a bug - but it sure isn't easy to understand.

TLDR; In short, its not a bug but a representation issue. I can fix the docs but I'd love to hear of ergonomic improvements you'd like to see.

Ordinal months versus Cardinal months

The full answer is that if you format the date then it formats what you are expecting:

iex> kr_date = Date.convert!(~D[2023-05-27], Cldr.Calendar.Korean)
~D[4356-05-08 Cldr.Calendar.Korean]
iex> Cldr.Date.to_string(kr_date, locale: :ko)
{:ok, "4356. 4. 8."}

You will very understandably say how on earth can the date struct have a value of 5 when its clearly 4 in the Korean lunar calendar.

The reason is implementation related. The Date.t/0 struct has no way to say that "month 2" is followed by "leap month 2". So I have to represent the months as counting from 1. So its better to say that the month number in aDate.t/0 is the ordinal month (the nth month) rather than the cardinal month number. I had a conversation with José about documenting that its the ordinal month number but he preferred to leave the number open to interpretation.

There are still bugs - just not that one

  1. There is however most definitely a bug - because if the month number is ordinal then in leap years I need to be able to create a month 13 and that currently isn't possible.
  2. Cldr.Date.to_string/2 is failing to format dates in leap months.

I will work on these two bugs tomorrow (I'm in UTC+8 timezone).

Possible improvements

  1. I need to make the documentation clearer that the Date.t for a Korean date (and also Chinese and Japanese) is the ordinal month number, not the cardinal.
  2. Take input on how to better represent leap months - or at least other functions that would make the development experience better.
kipcole9 commented 1 year ago

Cldr.Date.to_string/2 is failing to format dates in leap months.

Fixed now on ex_cldr_calendars on the cldr43 branch. Here's an example:

iex> Cldr.Date.to_string ~D[4356-03-01 Cldr.Calendar.Korean], locale: :ko
{:ok, "4356. 윤2. 1."}

# In the default :en locale
iex> Cldr.Date.to_string ~D[4356-03-01 Cldr.Calendar.Korean]
{:ok, "Mo2bis 1, 2023"}

Does that look closer to your expectations? (I am hopeful!)

kipcole9 commented 1 year ago

need to be able to create a month 13

Now also fixed in ex_cldr_calendars_lunisolar.

iex> Cldr.Date.to_string(~D[4356-13-01 Cldr.Calendar.Korean], locale: :ko)
{:ok, "4356. 12. 1."}

All these fixes and updates will be published next week.

nineclue commented 1 year ago

@kipcole9 Thanks for your reply.

I understood that the result were from the difference of ordinal and cardinal numbers and am really grateful for your efforts.

I still can't figure out how to find out how to find this year's Buddha's birthday (April 8th of every year in lunar calendar). Do I have to convert to string and parse the result ?

Really sorry to say this, but I googled and found ex_calendar_lunisolar module, but still can't figure out what modules are needed to use Cldr.Date.to_string function. backend_not_figured

kipcole9 commented 1 year ago

Happy to help. Here's some more information to get you started:

Configuration of an ex_cldr backend module

For ex_cldr and the related libraries, there is always a backend module involved that is generated at compile time. This backend module is typically the module upon which functions are call. An example is:

defmodule MyApp.Cldr do
  use Cldr,
    locale: ["en", "kr", "zh", "ja"],
    default_locale: "en",
    providers: [Cldr.Calendar, Cldr.Number, Cldr.DateTime]
end

Calling functions

Now we can call functions on the module MyApp.Cldr.

iex> MyApp.Cldr.Date.to_string(~D[4356-12-01 Cldr.Calendar.Korean], locale: :ko)
{:ok, "4356. 11. 1."}
iex> MyApp.Cldr.Date.to_string(~D[4356-12-01 Cldr.Calendar.Korean], locale: :ko, format: :long)
{:ok, "4356년 11월 1일"}

There is an alternative approach which is to call functions directly on the Cldr.Date module and passing the name of the backend module:

iex> Cldr.Date.to_string(~D[4356-13-01 Cldr.Calendar.Korean], locale: :ko, backend: MyApp.Cldr)
{:ok, "4356. 12. 1."}

The :backend option can be omitted only in the case that a configuration option :default_backend is configured. For example:

# in config.exs
config :ex_cldr, default_backend: MyApp.Cldr

But I recommend the first approach of calling functions directly on MyApp.Cldr (your configured backend module).

Buddha's birthday

Now we can represent Buddha's birthday. But since the ordinal month will be different depending on whether it is a leap year or not we need to make it conditional.

defmodule MajorEvents do
  def buddhas_birthday(korean_lunar_year) do
    if Cldr.Calendar.Korean.leap_year?(korean_lunar_year) do
      Date.new(korean_lunar_year, 5, 8, Cldr.Calendar.Korean)
    else
      Date.new(korean_lunar_year, 4, 8, Cldr.Calendar.Korean)
    end
  end
end

Here's an example:

iex> MajorEvents.buddhas_birthday 4356
{:ok, ~D[4356-05-08 Cldr.Calendar.Korean]}

Making the module more flexible

We can of course make this somewhat more flexible by destructuring the date:

defmodule MajorEvents do
  def buddhas_birthday(korean_lunar_year) when is_integer(korean_lunar_year) do
    if Cldr.Calendar.Korean.leap_year?(korean_lunar_year) do
      Date.new(korean_lunar_year, 5, 8, Cldr.Calendar.Korean)
    else
      Date.new(korean_lunar_year, 4, 8, Cldr.Calendar.Korean)
    end
  end

  def buddhas_birthday(%Date{year: korean_lunar_year, calendar: Cldr.Calendar.Korean}) do
    buddhas_birthday(korean_lunar_year)
  end

  def buddhas_birthday(%Date{calendar: Calendar.ISO} = date) do
    date
    |> Date.convert!(Cldr.Calendar.Korean)
    |> buddhas_birthday()
  end
end

And here's an example of its usage:

iex> MajorEvents.buddhas_birthday Date.utc_today()
{:ok, ~D[4356-05-08 Cldr.Calendar.Korean]}
iex> MajorEvents.buddhas_birthday ~D[4356-01-01 Cldr.Calendar.Korean]
{:ok, ~D[4356-05-08 Cldr.Calendar.Korean]}
kipcole9 commented 1 year ago

BTW, I will release the update for ex_cldr_calendars_lunisolar, along with updated to many ex_cldr libraries, by the end of Wednesday. That release will include the fixes noted above.

kipcole9 commented 1 year ago

I realise my implementation is not correct since leap month can be variable. I am working on an improvement that will ship with the new release. Hope to have the proof of concept done in the next 12 hours.

nineclue commented 1 year ago

Thanks for detailed information.

In Korea, lunar New years day, Buddha's birthday and lunar Thanks giving day (8-15) are national holidays. This year, those are 2023-01-22, 2023-05-27 and 2023-09-29. I wish there were easy way to find out.

kipcole9 commented 1 year ago

Thanks for pushing me to make this better.

I am working on an API to make creating dates much easier. I expect to have it finished today so I can ship with the rest of the ex_cldr updates.

iex> Cldr.Calendar.Korean.new(4356, 4, 8)
{:ok, ~D[4356-05-08 Cldr.Calendar.Korean]}
iex> Cldr.Calendar.Korean.new(4356, 3, 1)
{:ok, ~D[4356-03-01 Cldr.Calendar.Korean]}
iex> Cldr.Calendar.Korean.new(4356, {3, :leap}, 1)
{:ok, ~D[4356-04-01 Cldr.Calendar.Korean]}

I think that has much better ergonomics that match better what you're after. Comments and suggestions welcome of course.

kipcole9 commented 1 year ago

a zip file which contains numerous inserts clauses that matches lunar date to solar one from 1900/1/1 to 2200/12/31. field 'yun' matches leap month

I really appreciate this data and I'll leverage it for testing. Your data uses a different epoch for the lunar dates that my implementation. Do you have to know what the source of that epoch is? The epoch I am using is ~D[-2332-02-15] which my references say is the traditional founding date of the original Korean nation. If there is a different epoch in common use perhaps I should consider using that?

nineclue commented 1 year ago

Your idea of marking a leap month with tuple is great and your implementation look correct. Now we can create a lunar date with either integer or leap tuple and just convert to ISO date to get solar one.

nineclue commented 1 year ago

Traditional Korean year called Dangi is right, but we use Gregorian year combined with lunar month and date most of time. In other words, your implementation is theoretically correct but we usually think this year's (2023) New year or Thanksgiving day.

Converting year is simple add and subtract so I think it would not be a problem.

kipcole9 commented 1 year ago

I'm feeling much better now with this implementation. I need to do quite a lot of work on docs but the underlying lunisolar code I'm happy with. Using your examples you can now:

iex> Cldr.Calendar.Korean.new!(4356,8,15) |> Date.convert!(Calendar.ISO)
~D[2023-09-29]
iex> Cldr.Calendar.Korean.new!(4356,4,8) |> Date.convert!(Calendar.ISO)
~D[2023-05-27]
iex> Cldr.Calendar.Korean.new!(4356,1,1) |> Date.convert!(Calendar.ISO)
~D[2023-01-22]
iex> Cldr.Calendar.Korean.new!(4356,{3, :leap},1)
~D[4356-04-01 Cldr.Calendar.Korean]
iex> Cldr.Calendar.Korean.new!(4356,{4, :leap},1)
** (ArgumentError) cannot build date, reason: :invalid_date
    (ex_cldr_calendars_lunisolar 1.0.0) lib/cldr/calendar/korean.ex:52: Cldr.Calendar.Korean.new!/3

I'll work on docs first then the test data you provided.

kipcole9 commented 1 year ago

In other words, your implementation is theoretically correct but we think this year's (2023) New year or Thanksgiving day. Converting year is simple add and subtract so I think it would not be a problem.

Ah, thanks for the clarification. I could have asked my Korean colleagues but I try not to mix my day job with my development work :-)

I've now made the epoch configurable. So although I haven't fully tested it you could:

config :ex_cldr_calendars,
  korean_epoch: ~D[0001-01-01]

and it should then produce the results you've suggested.

nineclue commented 1 year ago

Your implementation look perfect! I think that I heard the data I provided might be different from official (national) calculation in several days. I think you'd better use the data just for reference.

kipcole9 commented 1 year ago

Everything to do with calendars is fun, interesting and completely makes my brain hurt. I really needed someone like you to push me harder to improve this. I really appreciate your patience and perseverance. I'll close this issue for now and push on with documentation and testing. I aim to have this library updated on hex.pm along with several others by tomorrow morning.

Please do let me know if there are other use cases or requirements that are more difficult than they should be.

nineclue commented 1 year ago

Thank you. You're really great.

I wish leap_month? function's first cycle argument get more documented and there exist a function that tells specific year's leap month, so we can convert ordinal month number to cardinal more easily.

What about leap_year? returning 0 if false and leap month number if not ?

kipcole9 commented 1 year ago

Can definitely do that - the functions already exist - I just need to surface them in the concrete calendar implementations. Will do right away. Great suggestions.

kipcole9 commented 1 year ago

Oh, also, leap_month/2 now takes a simple year and month so it's easier to comprehend. I will improve the docs for that too.

kipcole9 commented 1 year ago

I've done some refactoring, simplification and improvements to documentation and doc tests. I think its quite close now but any additional comments or suggestions would be warmly welcomed.

kipcole9 commented 1 year ago

Made the last changes I plan before release. I added lunar_month_of_year/{1, 3} to return the lunar month in the format that is compatible with new/3. And made cyclic_year/{1,3} public and documented since it does occur in some date time formatting patterns.

nineclue commented 1 year ago

I think your implementation fulfills all of the needs that I would need to use.

Thanks again for your kindness.

kipcole9 commented 1 year ago

Last last changes: