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 support for Israel (Jewish) Calander #289

Merged
merged 5 commits into from
Nov 19, 2024
Merged

Conversation

nadav7679
Copy link
Contributor

@nadav7679 nadav7679 commented Nov 12, 2024

Hey guys,

I added support for the Israeli stock exchange and Jewish calendar as in #142 .

The Israeli stock exchange follows Jewish calendar which isn't in sync with our usual Gregorian calendar, so every year the holiday dates shift (e.g. Passover eve was in April 5th 2023, but April 22nd on 2024). I tried to solve it using a call to an external API that answers if a given day is a holiday.

Please let me know what you think and provide lots of criticism! I'm trying to learn and want to progress to more issues.

Also, I noticed that two utility tests don't pass (unrelated to my changes):

test crates/RustQuant_time/src/utilities.rs - utilities::unpack_date (line 20) ... ignored
test crates/RustQuant_time/src/utilities.rs - utilities::leap_year_count (line 118) ... FAILED
Should I look into it?

@avhz
Copy link
Owner

avhz commented Nov 12, 2024

Thanks for your contribution, appreciate the effort.

This is a little tricky as I would rather avoid using API calls in general (the Yahoo Finance stuff will likely be deprecated prior to v1.0), as it relies on the API existing (and remaining secure) for the foreseeable future, and it is much slower than hard-coded holiday lists.

I am not experienced at all with other (non-Gregorian) calendars but my understanding is that there is unfortunately not a simple conversion formula between the Gregorian and Hebrew calendars ?

If you (and others) would find a Hebrew calendar useful, and are open to splitting the work, we could hard-code the holidays like we have done for the other calendars ?

This list seems relatively exhaustive up to Gregorian year 2050: https://en.wikipedia.org/wiki/Jewish_and_Israeli_holidays_2000%E2%80%932050

We could have functions for each major holiday, such as:

fn is_hebrew_weekend(date: Date) -> bool {
    date.weekday() == Friday || date.weekday() == Saturday
}

fn is_rosh_hashanah(date: Date) -> bool {
    let (y, m, d, _wd, _yd, _em) = unpack_date(date, false);

    if (
        (y == 1999 && d == 11 && m == Month::September) ||
        (y == 2000 && d == 30 && m == Month::September) ||
        (y == 2001 && d == 17 && m == Month::September) ||
        ...
    ) {
        return true;
    }
}

fn is_yom_kippur(date: Date) -> bool {
    let (y, m, d, _wd, _yd, _em) = unpack_date(date, false);

    if (
        (y == 1999 && d == 20 && m == Month::September) ||
        (y == 2000 && d == 9  && m == Month::October) ||
        (y == 2001 && d == 27 && m == Month::September) ||
        ...
    ) {
        return true;
    }
}

Admittedly this is much more code, but to avoid the API calls I think it's not too bad.

If there is actually a conversion formula, we could also do that instead.

Let me know what you think.

Cheers!

Edit: I also noticed this hebcal-rs repo, but it is not published to Crate.io (there is one with the same name, but it is a different repo), and it is GPL-2 licensed, meaning we can't use the code.

@avhz
Copy link
Owner

avhz commented Nov 12, 2024

And regarding the tests, the first is ignored so it doesn't 'pass', but the second one (utilities::leap_year_count) passes for me, are you on the latest commits ?

@nadav7679
Copy link
Contributor Author

nadav7679 commented Nov 13, 2024

@avhz I agree, I also thought that an API is inefficient but at first glance I thought it is the only solution. I believe I found a better solution now.

I used the heca-lib crate (which is a bit outdated but still works) to convert a given Gregorian date to a Jewish date and handled some idiosyncrasies manually (such as Adar 2 month). Then it was a simple matter of hardcoding all of the holidays expressed in the Jewish calendar and checking for a match.

I used the Tel-Aviv Stock Exchange for reference on which holidays are considered vacation, as some of the more religious holidays are valid trading days.

I believe it works well now, no API calls involved :)

Any comments on the current version? I'm still new to Rust so I'd like to improve in writing style, best practices, error handling and so on.

@nadav7679
Copy link
Contributor Author

Just noticed another thing that might cause problems.
The method is_business_day in trait Calendar uses a function called is_weekend (defined in utilities) that checks if the input day is Saturday or Sunday. In the Tel-Aviv Stock Exchange and Jewish calendar the weekends are Friday and Saturday. Sunday is a valid trading day.

How widespread is the use of is_business_day? Maybe it's best to deprecate it and always use the is_holiday method of the trait, which is only declarative. Another option is to make is_weekend a method instead of a function, so we could override it. We could also override is_business_day in structs of countries with different weekends, but I think this is quite confusing.

@avhz
Copy link
Owner

avhz commented Nov 16, 2024

To avoid depending on an un-maintained crate and adding chrono as a dependency, while still using your hard-coded holidays array, we can instead use icu:

impl IsraelCalendar {
    /// Hebrew weekend is Friday and Saturday,
    /// as opposed to Saturday and Sunday in the Gregorian calendar.
    fn is_weekend(&self, date: Date) -> bool {
        let wd = date.weekday();
        wd == Weekday::Friday || wd == Weekday::Saturday
    }
}

impl Calendar for IsraelCalendar {
    ...

    fn is_business_day(&self, date: Date) -> bool {
        !self.is_weekend(date) && !self.is_holiday(date)
    }

    fn is_holiday(&self, date: Date) -> bool {
        let (y, m, d, _, _, _) = unpack_date(date, false);
        let m = m as u8;

        let iso_date = icu::calendar::Date::try_new_iso_date(y, m, d)
            .expect("Failed to initialize ISO Date instance for constructing Hebrew date.");

        let hebrew_date = iso_date.to_calendar(icu::calendar::hebrew::Hebrew);
        let hebrew_month = hebrew_date.month().ordinal as u8;
        let hebrew_day = hebrew_date.day_of_month().0 as u8;

        JEWISH_HOLIDAYS.contains(&(hebrew_month, hebrew_day))
    }
}

Your unit tests still pass with this, and only requires adding one dependency that is actively developed.

Ideally we should clean-up the .expect(), but this would be my preferred implementation if we are going to add more dependencies.

Edit: This also handles your concern about Jewish weekends.

Edit #2: If you add some more tests based on the Tel Aviv exchange calendar, we can get this merged.

@nadav7679
Copy link
Contributor Author

nadav7679 commented Nov 19, 2024

I see, good point.
I updated the code to use the icu crate. I also had to add some logic to deal with specific cases, like Jewish independence day being postponed or not. There are specific, rigid rules for this.

Finally, I took many holiday dates from the Tel-Aviv Stock Exchange website and added them as tests (used ChatGPT for formatting). I believe this is exhaustive.

Hope this version is better!

Edit: I see now that some tests failed. Weird, they passed on my machine... Will look into it in a bit.

Comment on lines +96 to +104
let is_independence_or_memorial_day = match &(hebrew_month, hebrew_day, wd) {
(8, 3..=4, Weekday::Thursday) => true,
(8, 2..=3, Weekday::Wednesday) => true,
(8, 5, Weekday::Monday) => true,
(8, 6, Weekday::Tuesday) => true,
(8, 5, Weekday::Wednesday) => true,
(8, 4, Weekday::Tuesday) => true,
_ => false,
};

Check warning

Code scanning / clippy

match expression looks like matches! macro Warning

match expression looks like matches! macro
Comment on lines +96 to +104
let is_independence_or_memorial_day = match &(hebrew_month, hebrew_day, wd) {
(8, 3..=4, Weekday::Thursday) => true,
(8, 2..=3, Weekday::Wednesday) => true,
(8, 5, Weekday::Monday) => true,
(8, 6, Weekday::Tuesday) => true,
(8, 5, Weekday::Wednesday) => true,
(8, 4, Weekday::Tuesday) => true,
_ => false,
};

Check warning

Code scanning / clippy

match expression looks like matches! macro Warning

match expression looks like matches! macro
@avhz avhz merged commit 94d96cd into avhz:main Nov 19, 2024
7 of 9 checks passed
@avhz
Copy link
Owner

avhz commented Nov 19, 2024

Awesome, thanks for this PR :)

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