use neri-scheider algorithm in calendar

[dkuku]

This algorithm is O1 and it is over 2x faster than the current one.
I implemented it in elixir and this is the same logic. In the elixir pr the benchmarks are against elixir version of the current algorithm used in erlang.
It also works for negative dates which was not supported by the current algorithm, so the specs may need update but I keep it unchanged for now.

[CLAassistant]

All committers have signed the CLA.

[github-actions]

CT Test Results

Tests are running... https://github.com/erlang/otp/actions/runs/20011636102

Results for commit c87d145

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

• No CT logs found
• No HTML docs found
• No Windows Installer found

// Erlang/OTP Github Action Bot

[Maria-12648430]

Interesting... 😺

That said, I see that there are already tests for the affected functions in place, but IMO they're not too conclusive, they only test symmetry, and even that looks a little dubious to me. Could you provide some more tests (unit/common and maybe also property tests) that check if the computed results are actually correct? 🤔

[dkuku]

Interesting... 😺

That said, I see that there are already tests for the affected functions in place, but IMO they're not too conclusive, they only test symmetry, and even that looks a little dubious to me. Could you provide some more tests (unit/common and maybe also property tests) that check if the computed results are actually correct? 🤔

I did not check the existing tests, my bad. updated

[Maria-12648430]

I will take a closer look at the tests themselves tomorrow, but for now, note that you have to insert calls for the proptests in calendar_prop_SUITE.erl for them to be run.

[Maria-12648430]

Now that got me thinking 🤔 Turns out there is no year 0 in the Gregorian calendar, year 1BC is immediately followed by year 1AD 🤯

And while the calendar module doc states that the Gregorian calendar of this module "is extended back to year 0", IMO this simply doesn't work out. There is no year 0 in the Gregorian calendar, period. Year 0, if you would instead assume the astronomical calendar, would map to year 1BC in the Gregorian calendar. In any case, this would mean that "year 0" (1BC) is not a leap year.

@IngelaAndin (and maybe also @bjorng), what are your thoughts about this?

[Maria-12648430]

I see that is_leap_year(0) results in true, which considering what I said above would also be wrong?

[dkuku]

Maybe calling the current implementation gregorian is invalid because is uses astronomical and this is how it works currently:

 % iex 
Erlang/OTP 26 [erts-14.2.5] [source] [64-bit] [smp:14:14] [ds:14:14:10] [async-threads:1] [jit]

Interactive Elixir (1.16.2) - press Ctrl+C to exit (type h() ENTER for help)
iex(1)> :calendar.gregorian_days_to_date(0)
{0, 1, 1}
iex(2)> :calendar.gregorian_days_to_date(1)
{0, 1, 2}
iex(3)> :calendar.gregorian_days_to_date(58)
{0, 2, 28}
iex(4)> :calendar.gregorian_days_to_date(59)
{0, 2, 29}
iex(5)> 

[Maria-12648430]

Yeah, I know... the current implementation is according to ISO 8601:200x, which tl;dr boils down to astronomical year numbering (which makes more sense/is more practical than the gregorian with its cumbersome 1BC/1AD transition), but calls it "gregorian extended to 0"; the "... extended to 0" part only occurs in the module doc, though, everywhere else it is simply called Gregorian. Now, fixing the documentation would be no big issue I think, but unfortunately the astronomical-vs-gregorian confusion goes down to the function names, too...

However, that is kinda outside the scope of this PR. As far as this PR goes, I'm all good =)

[dkuku]

I will leave this for discussion. it's an existing bug.
Maybe introducing an astronomical_days_to_date and deprecating the gregorian/1 would be a solution ?

[dkuku]

For reference this is called date_from_iso_days in elixir.
elixir-lang/elixir@904adda

[Maria-12648430]

I consulted with @juhlig, and after some research we concluded that the documentation is correct and matches the implementation, and vice versa. A question that arose however is why there is a restriction for non-negative years. Admittedly, it is rare that you have to deal with such dates, but why restrict it at all?

[dkuku]

The previous implementation just did not support it - it required some tweaks.
I think this limitation can be removed - the algorithm supports negative dates just fine.
It is not a popular usecase probably - but I think it should be enabled even to allow solving riddles in erlang.
I can add tests for negative dates also. But it may be better to have a separate pr with the changes ?

[Maria-12648430]

This is for the OTP team to decide 🤷‍♀️ But seeing that there are many functions to change to consistely allow for negative years, plus their tests, plus documentation modifications, personally I'd opt for a separate PR.

[IngelaAndin]

I think @bjorng probably has some insight and otherwise I think @rickard-green is our time expert.