Add _IslamicMabimsLunar calendar class for MABIMS crescent visibility criteria#3603
Add _IslamicMabimsLunar calendar class for MABIMS crescent visibility criteria#3603akshiDhi wants to merge 35 commits into
Conversation
Indonesia: add 2026 Eid al-Fitr confirmed date (MAR 21) Signed-off-by: akshiDhi <jamkim2224@gmail.com>
Philippines: add 2026 Eid al-Fitr confirmed date (MAR 20) Signed-off-by: akshiDhi <jamkim2224@gmail.com>
Indonesia: add 2025-2026 Eid al-Adha confirmed dates Signed-off-by: akshiDhi <jamkim2224@gmail.com>
Philippines: add 2025-2026 Eid al-Adha confirmed dates Signed-off-by: akshiDhi <jamkim2224@gmail.com>
Signed-off-by: akshiDhi <jamkim2224@gmail.com>
Signed-off-by: akshiDhi <jamkim2224@gmail.com>
Signed-off-by: akshiDhi <jamkim2224@gmail.com>
Signed-off-by: akshiDhi <jamkim2224@gmail.com>
Removed holiday dates for 2025 and 2026 from the Philippines. Signed-off-by: akshiDhi <jamkim2224@gmail.com>
Signed-off-by: akshiDhi <jamkim2224@gmail.com>
Signed-off-by: akshiDhi <jamkim2224@gmail.com>
Signed-off-by: akshiDhi <jamkim2224@gmail.com>
Co-authored-by: ~Jhellico <KJhellico@users.noreply.github.com> Signed-off-by: akshiDhi <jamkim2224@gmail.com>
Signed-off-by: akshiDhi <jamkim2224@gmail.com>
Signed-off-by: akshiDhi <jamkim2224@gmail.com>
Signed-off-by: akshiDhi <jamkim2224@gmail.com>
Signed-off-by: akshiDhi <jamkim2224@gmail.com>
Signed-off-by: akshiDhi <jamkim2224@gmail.com>
Signed-off-by: akshiDhi <jamkim2224@gmail.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds _IslamicMabimsLunar — a MABIMS-based Islamic calendar subclass extending _IslamicLunar, introduces confirmed-year constants and Eid/ramadan lookup/shift logic (2001–2026), exports it from the package, and adds tests validating expected and fallback behaviors. ChangesMABIMS Islamic Calendar
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@holidays/calendars/islamic.py`:
- Around line 4283-4339: The class currently replaces the base _IslamicLunar
EID_AL_FITR_DATES and EID_AL_ADHA_DATES with 2001–2026-only dicts, causing
out-of-range years to lose fallbacks and these entries to be treated as
estimated by _get_holiday(); instead, merge (update) these verified maps into
the base EID_AL_FITR_DATES and EID_AL_ADHA_DATES rather than overwriting them,
and ensure entries from those maps are treated as confirmed by either adding
their years to the calendar’s confirmed/custom-year handling or setting the
estimated flag to False when _get_holiday() looks up dates from those specific
dicts (reference symbols: EID_AL_FITR_DATES, EID_AL_ADHA_DATES, class
_IslamicLunar, and method _get_holiday()).
In `@tests/calendars/test_islamic.py`:
- Around line 39-44: Update the tests to assert the returned confirmation flag
from the calendar methods instead of only the date: when iterating the output of
eid_al_fitr_dates() and eid_al_adha_dates() (used in tests at the assertions
around lines testing expected_date), unpack both values (date, is_estimated) and
assert that is_estimated is False for the verified years your calendar claims to
cover, in addition to asserting the expected_date is present.
- Around line 47-57: Add the 2006 dual-date edge case to the test mapping by
updating the eid_al_adha_dates fixture in tests/calendars/test_islamic.py to
include the MABIMS case where Eid al-Adha maps to two Gregorian dates in 2006
(i.e., add a 2006 entry that contains both Gregorian date values for that year)
and ensure the test normalizes/compares tuples/lists correctly so
tuple-normalization regressions are covered for the variable eid_al_adha_dates.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0fdce9cd-7af1-4870-badb-22beb3c44d89
📒 Files selected for processing (3)
holidays/calendars/__init__.pyholidays/calendars/islamic.pytests/calendars/test_islamic.py
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 3 files
Confidence score: 3/5
- There is a concrete regression risk in
holidays/calendars/islamic.py: redefiningEID_AL_FITR_DATESandEID_AL_ADHA_DATESwith only 2001–2026 values appears to replace the base_IslamicLunartables, which can remove Eid dates for years outside that range. - Because this is a user-visible holiday-calculation behavior change (severity 6/10, confidence 7/10), the PR carries some merge risk rather than being a low-risk cleanup.
- If this override is intentional, consider merging with a fallback/merge strategy so base-year coverage is preserved and existing behavior does not regress.
- Pay close attention to
holidays/calendars/islamic.py- date table overrides may truncate supported years and break Eid results outside 2001–2026.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/calendars/test_islamic.py`:
- Around line 67-75: The test test_fallback_to_base_outside_range currently
asserts is_estimated for entries from dates_1990 but omits checking dates_2030;
update the test to assert that all entries from both
self.calendar.eid_al_fitr_dates(1990) and self.calendar.eid_al_fitr_dates(2030)
have is_estimated True (e.g., iterate over both lists or concatenate them and
assert is_estimated for each tuple), keeping existing length checks and the test
name the same.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 161a778e-59ef-408d-863e-768bf91d6bfc
📒 Files selected for processing (2)
holidays/calendars/islamic.pytests/calendars/test_islamic.py
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
holidays/calendars/islamic.py (1)
1001-1002:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThese confirmed-year ranges are not wired into
_get_holiday().Lines 1001-1002 add plain
EID_*_DATES_CONFIRMED_YEARS, but_get_holiday()only reads the postfixed..._DATES_CONFIRMED_YEARS_{_CustomCalendar.CUSTOM_ATTR_POSTFIX}attribute on Lines 4147-4150. As written,super().eid_al_fitr_dates(year)/super().eid_al_adha_dates(year)still yieldis_estimated=True, so the MABIMS overrides keep returning estimated dates inside 2001-2026.Suggested fix
- confirmed_years = _normalize_tuple( - getattr( - self, f"{holiday}_DATES_CONFIRMED_YEARS_{_CustomCalendar.CUSTOM_ATTR_POSTFIX}", () - ) - ) + confirmed_years = _normalize_tuple( + getattr( + self, + f"{holiday}_DATES_CONFIRMED_YEARS_{_CustomCalendar.CUSTOM_ATTR_POSTFIX}", + getattr(self, f"{holiday}_DATES_CONFIRMED_YEARS", ()), + ) + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@holidays/calendars/islamic.py` around lines 1001 - 1002, The new constants EID_AL_FITR_DATES_CONFIRMED_YEARS and EID_AL_ADHA_DATES_CONFIRMED_YEARS aren't being used by _get_holiday() because that function only looks for attributes suffixed with _CustomCalendar.CUSTOM_ATTR_POSTFIX; update the code to either set the confirmed-year values under the expected attribute names (e.g., create EID_AL_FITR_DATES_CONFIRMED_YEARS_{_CustomCalendar.CUSTOM_ATTR_POSTFIX} and EID_AL_ADHA_DATES_CONFIRMED_YEARS_{_CustomCalendar.CUSTOM_ATTR_POSTFIX}) or change _get_holiday() to fallback to the plain constants when the postfixed versions are missing, and ensure that calls to super().eid_al_fitr_dates(year) and super().eid_al_adha_dates(year) will see is_estimated=False for years in the confirmed ranges.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@holidays/calendars/islamic.py`:
- Around line 4306-4326: _check_mabims_criteria currently uses a
declination-difference heuristic and ignores LONGITUDE, so replace it with a
proper topocentric altitude check at local sunset: use
_get_solar_lunar_positions to obtain geocentric RA/Dec (or ecliptic long/lat
converted to RA/Dec), compute local sidereal time from days_since_j2000 and
self.LONGITUDE, derive the moon's local hour angle and then convert RA/Dec +
observer lat (self.LATITUDE) to topocentric altitude; compute the sunset time by
solving for the Sun's hour angle at altitude ≈ -0.833° (or use an existing
sunset routine) and evaluate the moon altitude at that sunset time, then apply
the MABIMS thresholds (e.g., altitude >= 3.0° and elongation >= 6.4°).
Alternatively, if you cannot implement a validated topocentric sunset,
remove/disable algorithmic checks in _check_mabims_criteria and use explicit
trusted dates in _CustomIslamicHolidays / CONFIRMED_YEARS instead.
- Around line 4286-4324: The module is missing the math import causing NameError
when calling _get_solar_lunar_positions or _check_mabims_criteria; add "import
math" at the top of the module (near other imports) so all uses of math.sin,
math.cos, math.radians, math.degrees, math.acos, math.asin, etc. in
_get_solar_lunar_positions and _check_mabims_criteria resolve correctly.
---
Duplicate comments:
In `@holidays/calendars/islamic.py`:
- Around line 1001-1002: The new constants EID_AL_FITR_DATES_CONFIRMED_YEARS and
EID_AL_ADHA_DATES_CONFIRMED_YEARS aren't being used by _get_holiday() because
that function only looks for attributes suffixed with
_CustomCalendar.CUSTOM_ATTR_POSTFIX; update the code to either set the
confirmed-year values under the expected attribute names (e.g., create
EID_AL_FITR_DATES_CONFIRMED_YEARS_{_CustomCalendar.CUSTOM_ATTR_POSTFIX} and
EID_AL_ADHA_DATES_CONFIRMED_YEARS_{_CustomCalendar.CUSTOM_ATTR_POSTFIX}) or
change _get_holiday() to fallback to the plain constants when the postfixed
versions are missing, and ensure that calls to super().eid_al_fitr_dates(year)
and super().eid_al_adha_dates(year) will see is_estimated=False for years in the
confirmed ranges.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fd71cd61-e1f4-4052-a151-da5cb8198bf9
📒 Files selected for processing (1)
holidays/calendars/islamic.py
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
holidays/calendars/islamic.py (1)
1001-1002:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the confirmed-year ranges on the MABIMS subclass, not the shared base.
_get_holiday()readsEID_*_DATES_CONFIRMED_YEARSfromself, so putting(2001, 2026)on_IslamicLunarflips the estimated flag for every generic Islamic-calendar consumer in that range. That suppresses downstream"estimated"labels well outside this feature. Move these ranges to_IslamicMabimsLunar(or keep explicit confirmed dates there) so only the MABIMS calendar claims those years are confirmed.Suggested direction
class _IslamicLunar: - EID_AL_FITR_DATES_CONFIRMED_YEARS = (2001, 2026) - EID_AL_ADHA_DATES_CONFIRMED_YEARS = (2001, 2026) - class _IslamicMabimsLunar(_IslamicLunar): + EID_AL_FITR_DATES_CONFIRMED_YEARS = (2001, 2026) + EID_AL_ADHA_DATES_CONFIRMED_YEARS = (2001, 2026)Based on learnings:
CONFIRMED_YEARSrepresents years where the underlying calculation has actually been verified, and Islamic holiday dates should only be treated as confirmed when officially verified.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@holidays/calendars/islamic.py` around lines 1001 - 1002, The confirmed-year tuples EID_AL_FITR_DATES_CONFIRMED_YEARS and EID_AL_ADHA_DATES_CONFIRMED_YEARS should not live on the shared _IslamicLunar base because _get_holiday() reads them from self and that causes non‑MABIMS calendars to be treated as "confirmed" for 2001–2026; move those constants (or their explicit confirmed-date lists) into the MABIMS-specific subclass _IslamicMabimsLunar so only _IslamicMabimsLunar reports those years as confirmed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@holidays/calendars/islamic.py`:
- Around line 1001-1002: The confirmed-year tuples
EID_AL_FITR_DATES_CONFIRMED_YEARS and EID_AL_ADHA_DATES_CONFIRMED_YEARS should
not live on the shared _IslamicLunar base because _get_holiday() reads them from
self and that causes non‑MABIMS calendars to be treated as "confirmed" for
2001–2026; move those constants (or their explicit confirmed-date lists) into
the MABIMS-specific subclass _IslamicMabimsLunar so only _IslamicMabimsLunar
reports those years as confirmed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7c138b08-8927-42a6-a1a9-1e01c7c7205f
📒 Files selected for processing (2)
holidays/calendars/islamic.pytests/calendars/test_islamic.py
… MABIMS generator
|
i updated the implementation following the feedback. |
| # Singapore coordinates (primary MABIMS Hisab reference). | ||
| SINGAPORE_LAT = 1.3521 | ||
| SINGAPORE_LON = 103.8198 | ||
| SINGAPORE_ELEV = 15.0 # meters |
There was a problem hiding this comment.
Why is Singapore used as the reference location? Please provide the source that defines it as the MABIMS reference point.
There was a problem hiding this comment.
in issue #2334 ppsyrius mentions singapore as reference location and it is the only MABIMS member that relies exclusively on Hisāb (astronomical calculation) since 1974 making it a consistent reference for this issue and other MAIBMS members use Ru'yah which depends on human sightings and weather as their final determinantion method. Singapore's calculation gives the MABIMS result that all member countries typically follow.
Source :
https://www.muslim.sg/articles/ramadan-countdown-unity-in-diversity
https://www.muis.gov.sg/resources/media-releases/announcement-by-mufti-for-the-month-of-syawal-1446h--2025--for-singapore
|
|
||
| def check_mabims_visibility(ts, eph, observer, check_date: date) -> bool: | ||
| """Check if crescent moon meets MABIMS criteria at Singapore sunset.""" | ||
| t = ts.utc(check_date.year, check_date.month, check_date.day, 11, 0, 0) # ~sunset UTC+8 |
There was a problem hiding this comment.
Why not the exact sunset time?
There was a problem hiding this comment.
yeah i fixed it. Now it uses almanac.sunrise_sunset() from skyfield to calculate the exact sunset time for Singapore. Falls back to approximate time only if sunset cannot be found in the expected window
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
|
General advice on the algorithm:
|
Signed-off-by: akshiDhi <jamkim2224@gmail.com>
Added type annotations for various date attributes in the _IslamicLunar class. Signed-off-by: akshiDhi <jamkim2224@gmail.com>
Removed unused import of the math module. Signed-off-by: akshiDhi <jamkim2224@gmail.com>
Signed-off-by: akshiDhi <jamkim2224@gmail.com>
…ations Signed-off-by: akshiDhi <jamkim2224@gmail.com>
|
I have updated the PR based on your feedback:
|
If you're already using Skyfield, you should take advantage of its features, including calculating arrays of values all at once, rather than one by one. I mean something like times, events = almanac.find_discrete(t0, t1, almanac.moon_phases(eph))
new_moons = [
moment for moment, phase in zip(times, events) if phase == 0
]for the entire timeframe. |
|
thank you ! I updated the script to take advantage of Skyfield's vector arrays. (i chunked the find_discrete() calls year-by-year in a loop to avoid a Skyfield memory error) |
| def get_approximate_date(self, h_year: int, h_month: int) -> date: | ||
| months_since_epoch = (h_year - 1) * 12 + (h_month - 1) | ||
| approx_days = int(months_since_epoch * MEAN_SYNODIC_MONTH) | ||
| return HIJRI_EPOCH + timedelta(days=approx_days) | ||
|
|
||
| def get_new_moon_date(self, approximate_date: date) -> date: | ||
| """Find the closest precalculated new moon (conjunction) near the given date.""" | ||
| # Find the new moon with the minimum absolute difference in days | ||
| return min(self.new_moons, key=lambda nm: abs((nm - approximate_date).days)) |
There was a problem hiding this comment.
What's the point of all this? You can just cycle through the calculated new moon moments.
|
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/calendar/mabims_generator.py">
<violation number="1" location="scripts/calendar/mabims_generator.py:110">
P2: `get_new_moon_date` directly indexes `self.new_moons` without checking that the computed index is within the precomputed list. If the generation range is ever extended to earlier Hijri years, Python's negative list indexing will silently return an unrelated new-moon date from the end of the table, corrupting the generated calendar data. Adding an explicit bounds check (or an `assert`) would make the script resilient to range changes and prevent silent failures.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| def get_new_moon_date(self, h_year: int, h_month: int) -> date: | ||
| """Get the exact precalculated new moon (conjunction) for a given Hijri month.""" | ||
| months_since_epoch = (h_year - 1) * 12 + (h_month - 1) | ||
| return self.new_moons[months_since_epoch - self.month_offset] |
There was a problem hiding this comment.
P2: get_new_moon_date directly indexes self.new_moons without checking that the computed index is within the precomputed list. If the generation range is ever extended to earlier Hijri years, Python's negative list indexing will silently return an unrelated new-moon date from the end of the table, corrupting the generated calendar data. Adding an explicit bounds check (or an assert) would make the script resilient to range changes and prevent silent failures.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/calendar/mabims_generator.py, line 110:
<comment>`get_new_moon_date` directly indexes `self.new_moons` without checking that the computed index is within the precomputed list. If the generation range is ever extended to earlier Hijri years, Python's negative list indexing will silently return an unrelated new-moon date from the end of the table, corrupting the generated calendar data. Adding an explicit bounds check (or an `assert`) would make the script resilient to range changes and prevent silent failures.</comment>
<file context>
@@ -85,24 +90,30 @@ def __init__(self) -> None:
+ def get_new_moon_date(self, h_year: int, h_month: int) -> date:
+ """Get the exact precalculated new moon (conjunction) for a given Hijri month."""
+ months_since_epoch = (h_year - 1) * 12 + (h_month - 1)
+ return self.new_moons[months_since_epoch - self.month_offset]
def check_mabims_visibility(self, check_date: date) -> bool:
</file context>
| return self.new_moons[months_since_epoch - self.month_offset] | |
| idx = months_since_epoch - self.month_offset | |
| if not 0 <= idx < len(self.new_moons): | |
| raise ValueError( | |
| f"Hijri month ({h_year}-{h_month}) is outside the precomputed new-moon range." | |
| ) | |
| return self.new_moons[idx] |
2 participants
Proposed change
Uses skyfield to calculate moon positions at Singapore sunset
Applies MABIMS 2021 criteria (altitude >= 3°, geocentric elongation >= 6.4°)
Generates pre-computed date tables covering 1925-2052 for 7 Islamic holidays (Eid al-Fitr, Eid al-Adha, Hijri New Year, Isra and Miraj, Mawlid, Nuzul al-Quran, Ramadan Beginning)
Dates are based on Singapore's MUIS (Majlis Ugama Islam Singapura)
astronomical calculations (Hisab) as the primary reference source,
covering Eid al-Fitr and Eid al-Adha from 2001 to 2026.
Related to #2334
Type of change
holidaysfunctionality in general)Checklist
make checklocally; all checks and tests passed.