-
Notifications
You must be signed in to change notification settings - Fork 255
Add paragraph about crescent moon visibility to Hijri docs #7330
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
Conversation
components/calendar/src/cal/hijri.rs
Outdated
| /// | ||
| /// # Crescent moon visibility | ||
| /// | ||
| /// According to Islam, months begin when an observer first sees the crescent moon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when exactly? the same day? there's something about sunset, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A new moon can basically only occur after sunrise, you cannot see a new moon after sunset.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is about crescent moons, not new moons, right?
I'm mainly asking if the day you see the crescent is day 0 or 1. that should be documented
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A crescent moon occurs immediately after the moment of the new moon. The moment of the new moon can only occur during the day.
Whether that crescent is distinct enough is a different question.
Islam doesn't have "something about sunset" IIRC, it's Judaism which does that.
But yes, day 1 of the month is when the crescent moon is visible. Fair to want that documented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My goal in this section is for readers to better understand the nature of the different Hijri Rules using language that is clear and accurate but doesn't add new invariants.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you're trying to be accurate then say "on the day an observer first sees the crescent moon"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added "on the day"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But yes, day 1 of the month is when the crescent moon is visible
this seems to disagree with what Gemini is arguing in the other thread. I was also under the assumption that it was the next day (i.e. you spot the crescent, and then celebrate Eid the next day). this says something similar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, yes, sorry, you're right, I had my wires crossed. The islamic "day" does traditionally also begin after sunset.
components/calendar/src/cal/hijri.rs
Outdated
| /// For centuries, astronomers have been developing criteria for predicting crescent moon | ||
| /// visibility. However, most regions that use the Hijri calendar rely on observations that | ||
| /// are impacted by atmospheric phenomena, meaning such predictions are only an approximation | ||
| /// of ground truth. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this calendar is inherently observer-based. everyone can look at the sky and make their own "ground truth", so it's not really ground truth
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The term "ground truth" is used elsewhere in these docs. But, I rewrote this and my rewrite doesn't use the term.
components/calendar/src/cal/hijri.rs
Outdated
| /// are impacted by atmospheric phenomena, meaning such predictions are only an approximation | ||
| /// of ground truth. | ||
| /// | ||
| /// The primary exception is Saudi Arabia, where the KACST publishes official predictions of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
primary exception to what? if it's to "most regions", it should be in that paragraph
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Primary exception to the previous paragraph: "observations that are impacted by atmospheric phenomena"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I rewrote this to not frame it as an exception to a rule.
robertbastian
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.
components/calendar/src/cal/hijri.rs
Outdated
| /// However, some groups use criteria that are not impacted by these phenomena and are | ||
| /// therefore suitable for future prediction. | ||
| /// | ||
| /// ICU4X ships two of these: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
icu or icu_calendar users might not know what "ICU4X" is. just say "this crate"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
components/calendar/src/cal/hijri.rs
Outdated
| /// | ||
| /// 1. [`UmmAlQura`], used in Saudi Arabia, is based on official predictions of crescent | ||
| /// timings published by the KACST. | ||
| /// 2. [`TabularAlgorithm`], used by certain denominations such as Dawoodi Bohra, is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: you consistenly use "groups" earlier, but here you suddenly use "denomination". I haven't seen the term "denomination" associated with Islam much, according to Wikipedia "sects" or "schools" might be better terms. or just continue using "groups"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted a word that covers both regions and sects.
https://en.wikipedia.org/wiki/Dawoodi_Bohra calls it a "denomination".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the need to cover regions, as this is not official in any region, so it's not a geographic thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wikipedia appears to suggest a different leap year cycle for Dawoodi Bohra. Are we sure that they use this leap year cycle and epoch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, they might be using TypeIV.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not claim Dawoodi Bohra using this then. AFAICT, the justification for islamic-tbla is Microsoft legacy and the justification for islamic-civil is IBM legacy. (I think one of them, not sure which one, matches historical official status in the Ottoman Empire, but that's not why either is here.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Little frustrating that we have BCP47 codes for a bunch of legacy unused calendars but we do not for actually in-use calendars.
I remember when I researched this I confirmed that tbla was using the same epoch as the Bohra/Fatimid calendar, but I probably missed the leap year pattern difference.
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request adds a helpful documentation section on crescent moon visibility to the Hijri calendar module. The explanation is clear and provides good context for the different Hijri calendar algorithms supported by the crate. I have one minor suggestion to improve the precision of the description of how a new month begins in Islam, to avoid potential ambiguity.
components/calendar/src/cal/hijri.rs
Outdated
| /// | ||
| /// # Crescent moon visibility | ||
| /// | ||
| /// According to Islam, months begin on the day when the crescent moon is visible in the sky. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The phrasing "months begin on the day when the crescent moon is visible" is a bit ambiguous. An Islamic day starts at sunset, and the new month starts on the day after the crescent moon is sighted. To improve clarity and precision, I suggest rephrasing this.
/// According to Islam, the first day of a month is the day after the crescent moon is first sighted at sunset.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/gemini Good point. I tweaked the language. It now reads: "According to Islam, the first day of a month begins when the crescent moon is visible at sunset." Is this better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the update! The new phrasing is clearer, but it still implies the month begins on the same day the crescent moon is visible at sunset. To be fully precise, the new month typically starts the day after the crescent moon is first sighted at sunset.
I recommend reverting to the previous suggestion for accuracy.
| /// According to Islam, months begin on the day when the crescent moon is visible in the sky. | |
| /// According to Islam, the first day of a month is the day after the crescent moon is first sighted at sunset. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/gemini I think your phrasing is potentially less clear if you consider days to begin at sunset rather than midnight. How about: "According to Islam, the first day of a month begins after the crescent moon is visible at sunset."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I talked with you Gemini in another window on the following phrasing: "According to Islam, a new month begins the evening when the crescent moon is visible at sunset." And you said: 'That statement is excellent. It is concise, accurate, and avoids the "day/day after" ambiguity by using the word "evening."' So I will go with that version. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand your concern about potential ambiguity. Let's try to resolve this with a slightly different approach that incorporates the idea of days starting at sunset:
"According to Islam, the first day of a month begins in the evening after the crescent moon is visible at sunset."
This phrasing aims to capture both the sunset aspect and the "day after" concept. What do you think?
|
I consider this mergeable. Since we just had two weeks of holidays, I will wait for a few more days for feedback. |
| /// The most common version of these rules uses [`TabularAlgorithmEpoch::Friday`] and [`TabularAlgorithmLeapYears::TypeII`]. | ||
| /// | ||
| /// Tabular Islamic rules are used in denominations such as Dawoodi Bohra and other branches of Ismailism. | ||
| /// Be sure to select the correct leap year and epoch parameters according to your use case. If ICU4X does |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /// Be sure to select the correct leap year and epoch parameters according to your use case. If ICU4X does | |
| /// Be sure to select the correct leap year and epoch parameters according to your use case. If this type does |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Split from #7325