Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Basic Keviyah module for Hebrew Calendar #4500

Merged
merged 1 commit into from
Dec 28, 2023

Conversation

Manishearth
Copy link
Member

@Manishearth Manishearth commented Dec 28, 2023

Part of #3933

This implements a module that contains efficient Hebrew calendrical calculations using the Four Gates method. It is more efficient than the book code (which is a good demonstration of some algorithms but has not been mathematically simplified, or cached in any way) and works off of the intermediate notion of Keviyah, which is a cacheable quantity that characterizes the type of year.

This doesn't yet plug this code into icu_calendar, but it does test that it has exactly the same behavior as the book code.

I do link the sources, but I hope that all the math here is explained adequately in the comments. The sources should only need to be consulted for the actual values of the constants. Please let me know if this is not the case and I'll document further.

A thing I'm not yet sure on (which doesn't block this PR) is what should actually be cached in the icu_calendar::Hebrew YearInfo. We basically have two options:

  • Cache just the keviyah, or perhaps YearType/StartOfYear/is_leap. This is quite compact, but conversion to ISO will require computation of the molad. molad_details is not a particularly expensive method to call; but it's not completely cheap.
  • Cache the keviyah and the weeks_since_beharad. This has danger of blowing the bounds of an i64, though we can probably optimize it by instead storing the RataDie of the beginning of the week, stuffing the index of the Keviyah in the %7 of the value, and calculating is_leap from the year.

The implementation of this module makes it rather straightforward to hop between either strategy, so it's not a big deal yet. We can benchmark once we implement this.

@sffc
Copy link
Member

sffc commented Dec 28, 2023

  • Cache the keviyah and the weeks_since_beharad. This has danger of blowing the bounds of an i64, though we can probably optimize it by instead storing the RataDie of the beginning of the week, stuffing the index of the Keviyah in the %7 of the value, and calculating is_leap from the year.

I'm not a fan of caching weeks_since_beharad because:

  1. It can be computed using a single integer division. This doesn't scare me like the new-moon calculations do.
  2. Not excited about the YearInfo cache having a variable that doesn't have an upper bound (well, other than a RataDie)

// Most of the math can be found on Wikipedia as well,
// at <https://en.wikipedia.org/wiki/Hebrew_calendar#The_four_gates>

// A note on time notation
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion (optional): make these module doc comments //! so that they render on rustdoc and educate more people and are easier to read

Comment on lines +56 to +57
/// Calculate the number of months preceding the molad Tishri for a given hebrew year (Tishri is the first month)
fn months_preceding_molad(h_year: i32) -> i32 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Calculate the number of months preceding the molad Tishri for a given hebrew year (Tishri is the first month)
fn months_preceding_molad(h_year: i32) -> i32 {
/// Calculate the number of months between the Hebrew epoch and Tishri for a given hebrew year (Tishri is the first month)
fn months_preceding_molad(h_year: i32) -> i32 {


/// Conveniently create a constant for a ḥalakim (by default in 1-indexed notation). Produces a constant
/// that tracks the number of ḥalakim since the beginning of the week
macro_rules! ḥal {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Observation: non-ASCII identifier. At first I thought I had dust on my screen.

///
/// This is 29-12-793 in zero-indexed notation. It is equal to 765433ḥal.
/// From Adjler Appendix A
const HEBREW_LUNATION_TIME: i32 = ḥal!(0-indexed 29-12-793);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thought: HEBREW_LUNATION_TIME is a floating duration, whereas MOLAD_BEHERAD_OFFSET is a specific instant, but they are both generated using the ḥal! macro.

/// the same as the Molad Beherad.
///
/// (note that the molad Beherad occurs on standard Sunday, but because it happens after 6PM it is still Hebrew Monday)
const HEBREW_CALENDAR_EPOCH: RataDie = crate::julian::fixed_from_julian_book_version(-3761, 10, 7);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thought: fixed_from_julian_book_version takes a Julian year/month/day but you are giving it a Julian year/month and a Hebrew day.


// The molad tishri expressed in parts since the beginning of the week containing Molad of Beharad
// Formula from Adjler Appendix A
let molad = MOLAD_BEHERAD_OFFSET as i64 + months_preceding as i64 * HEBREW_LUNATION_TIME as i64;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thought: I think this won't overflow because the largest molad on an h_year: i32 is about 2^54. Maybe worth documenting.

// The +1 is because our match statement is also 1-indexed
// and we want to have this match statement match resources that list
// these year types (both Adjler and Wikipedia).
match ((h_year - 1) % 19) + 1 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thought: this would work just fine with out the -1 +1 because the only difference is whether the 19th year is 0 or 19 but it is captured in the _ case either way

Comment on lines +429 to +431
/// Perform the four gates calculation, giving you the Keviyah for a given year type and
/// the ḥalakim-since-beginning-of-week of its molad Tishri
fn keviyah_for(year_type: MetonicCycleType, ḥalakim: i32) -> Keviyah {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"year type" was previously defined to mean Deficient/Regular/Complete

Suggested change
/// Perform the four gates calculation, giving you the Keviyah for a given year type and
/// the ḥalakim-since-beginning-of-week of its molad Tishri
fn keviyah_for(year_type: MetonicCycleType, ḥalakim: i32) -> Keviyah {
/// Perform the four gates calculation, giving you the Keviyah for a given metonic type and
/// the ḥalakim-since-beginning-of-week of its molad Tishri
fn keviyah_for(metonic_type: MetonicCycleType, ḥalakim_of_tishri: i32) -> Keviyah {

@sffc
Copy link
Member

sffc commented Dec 28, 2023

  • Cache the keviyah and the weeks_since_beharad. This has danger of blowing the bounds of an i64, though we can probably optimize it by instead storing the RataDie of the beginning of the week, stuffing the index of the Keviyah in the %7 of the value, and calculating is_leap from the year.

I'm not a fan of caching weeks_since_beharad because:

  1. It can be computed using a single integer division. This doesn't scare me like the new-moon calculations do.
  2. Not excited about the YearInfo cache having a variable that doesn't have an upper bound (well, other than a RataDie)

Actually weeks_since_beharad is just like a RataDie, and in fact the largest value of weeks_since_beharad is quite small, only about 2 digits wider than an i32, so there should be plenty of bits to store everything you need in the i64.

@Manishearth
Copy link
Member Author

Not excited about the YearInfo cache having a variable that doesn't have an upper bound (well, other than a RataDie)

Well, it's going to be bound by RataDie, it's always smaller than RD/7.

Actually weeks_since_beharad is just like a RataDie, and in fact the largest value of weeks_since_beharad is quite small, only about 2 digits wider than an i32, so there should be plenty of bits to store everything you need in the i64.

Yeah though I think a more clever way to do it to really match up with RD bounds for large years is to store sunday_before_ny as an RD and then store the keviyah in the % 7 part.

But let's see if this is actually needed.

@Manishearth Manishearth merged commit afb78dd into unicode-org:main Dec 28, 2023
29 checks passed
@Manishearth
Copy link
Member Author

It can be computed using a single integer division. This doesn't scare me like the new-moon calculations do.

it's technically a small multiplication, a small division, and then in i64 a multiplication, addition, and division

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants