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

Islamic Calendars incorrect dayOfYear #2740

Closed
MohsenAlyafei opened this issue Dec 19, 2023 · 16 comments · Fixed by #2743
Closed

Islamic Calendars incorrect dayOfYear #2740

MohsenAlyafei opened this issue Dec 19, 2023 · 16 comments · Fixed by #2743

Comments

@MohsenAlyafei
Copy link

The dayOfYear property does not return the day of the year for the given Islamic calendars' dates but rather to the day of year for the corresponding gregorian year.

For example: The first day of this islamic year 1445 AH returns day 248 of the islamic year which is incorrect.

This doesn't seem to be correct specification.

This is also the case with the Persian calendar.

Currently to get the dayOfYear is to use the "since" or "until" methods to calculate the day from the start of the given Islamic year.

The weekOfYear is also incorrect for the Islamic calendars and shows the ISO week number instead !

@khawarizmus
Copy link
Contributor

I am not sure if this is against the specification. however I noticed that if you are creating your dates from strings then Temporal assumes them to be in Gregorian and creates the equivalent Hijri date.

for example:

const date = Temporal.ZonedDateTime.from("1441-12-01T00:00:00+00:00[UTC][u-ca=islamic-umalqura]")
// this will create a the Gregorian date 1441-12-01 and convert the internal values to Hijri

console.log(date.year) // equivalent Hijri year 845
console.log(date.daysInMonth) // 30
console.log(date.month) // equivalent Hijri year 7

However if you set the values explicitly it yields what you expect:

// this is probably what you want
const date2 = Temporal.ZonedDateTime.from({
timeZone: "UTC",
year: 1441,
month: 12,
day: 1,
calendar: "islamic-umalqura"
})

console.log(date2.year) // 1441
console.log(date2.daysInMonth) // 29
console.log(date2.month) // 12

Also note that the toString method always prints the Gregorian date not the Hijri date.

console.log(date2.toString()) // "2020-07-22T00:00:00+00:00[UTC][u-ca=islamic-umalqura]"

@khawarizmus
Copy link
Contributor

As for the weekOfYear it can only show the Gregorian ISO week number because to my knowledge we don't have an ISO Hijri calendar standard that we can use (not sure about Persian and other calendars). Maybe someone with more familiarity on the topic can shed some light on this.

@MohsenAlyafei
Copy link
Author

MohsenAlyafei commented Dec 19, 2023

I am not sure if this is against the specification. however I noticed that if you are creating your dates from strings then Temporal assumes them to be in Gregorian and creates the equivalent Hijri date.

for example:

const date = Temporal.ZonedDateTime.from("1441-12-01T00:00:00+00:00[UTC][u-ca=islamic-umalqura]")
// this will create a the Gregorian date 1441-12-01 and convert the internal values to Hijri

console.log(date.year) // equivalent Hijri year 845
console.log(date.daysInMonth) // 30
console.log(date.month) // equivalent Hijri year 7

However if you set the values explicitly it yields what you expect:

// this is probably what you want
const date2 = Temporal.ZonedDateTime.from({
timeZone: "UTC",
year: 1441,
month: 12,
day: 1,
calendar: "islamic-umalqura"
})

console.log(date2.year) // 1441
console.log(date2.daysInMonth) // 29
console.log(date2.month) // 12

Also note that the toString method always prints the Gregorian date not the Hijri date.

console.log(date2.toString()) // "2020-07-22T00:00:00+00:00[UTC][u-ca=islamic-umalqura]"

No. The date is passed as an object with proper year, month, and date and correct Islamic calendar. i.e. like your second example.

True, a date string will be interpreted as ISO Gregory date and then converted to Islamic; which is not the issue in question.

The question or issue is about the dayOfYear.

In your second example above if you do:
console.log(date2.dayOfYear) it will give 185 which is incorrect. The day of year for the Islamic date 1/12/1441 AH is 327 which is only 28 days before islamic year end.

The day of year number outputted is actually for the Gregory date 22 July 2020. This is incorrect day-of-year for the Islamic date.

When using a different calendar, it is expected that the properties relate to that calendar not to any other calendar else be left undefined.

True that there is no iso week defined for a hijri date but week numbers exist for Islamic calendars starting from the 1st day of the islamic year with a total of 51 weeks.

@haikyuu
Copy link

haikyuu commented Dec 20, 2023

we have no ISO Hijri calendar

Here are some special differences from Gregorian calendar:

  • It varies with location, but it's a +- 1 day difference between any two calendars. For example, it can be 1 ramadan in Morocco while it's 2 ramadan in europe. Or the length of a given month in a given year is 29 or 30 depending on location. This is because the calendar is lunar, and there are different calculation methods.
  • Start of day is different: the day starts at sunset! So the start of the day actually varies location-wise but also in different times in the "Solar" calendar.

One example of the practical use of this is that there is a special prayer Tarawih that's usually be performed in Ramadan (a month). So 1st Ramadan @20:00 in Morocco is actually just a couple of hours after sunset of the previous gregorian day !! and in order for a program to check if a given day should have Tarawih is to know at which time the Hijri day actually starts.

Happy to discuss more or provide more clarification on these topics. Here is a quite popular and maintained library that does the astronomical calculations (based on location) in order to provide precise timings for prayers. The one interesting thing that should be included IMHO in Temporal is sunset, since there is no way to know the precise start of the day otherwise.

Also, another consideration in API usage is to allow a user to provide an actual/default location to be used by the algorithm.

I think this is the way forward if we want to be inclusive to Hijri calendar that 1.8 billion of people actually rely on every day 🩷

@MohsenAlyafei
Copy link
Author

The information provided are incorrect by the Temporal dates for Islamic calendars.

An example of 1st day of 1st month of this year 1445 AH.

const d = Temporal.PlainDate.from({year:1445, month:1, day:1, calendar: 'islamic-umalqura'});
console.log(d);

The ouput:

Temporal.PlainDate {}
calendarId: "islamic-umalqura"
day: 1
dayOfWeek: 3
dayOfYear: 248   // <== Incorrect day of year should be 1
daysInMonth: 29
daysInWeek: 7
daysInYear: 354  // <== Correct
era: "ah"
eraYear: 1445
inLeapYear: true // incorrect should be false because it is a 354 day’s year
month: 1
monthCode: "M01"
monthsInYear: 12
weekOfYear: 29    // should be 1
year: 1445
yearOfWeek: 2023  // should be 1445

@khawarizmus
Copy link
Contributor

khawarizmus commented Dec 21, 2023

@ptomato @justingrant Any comments on the above? are you guys aware of these issues?

@MohsenAlyafei
Copy link
Author

@haikyuu
The concern is that users will trust that when they specify a different calendar other than Gregory or iso that the data relates to the calendar they requested.
Today they will get mixed data partly to their calendar and part to the Gregory calendar.
Logically, why should they be given data about a calendar they did not ask for or requested.
This is a shortfall in API.
It is safer to return a property value as 'undefined' rather than giving misleading information. A future improvement can fill the undefined value with correct value.

If this is allowed now and the API gets implemented it will be difficult to fix later without having to create new properties.

We have waited over 12 years to get a proper date API under javascript, so we can wait for few months to get it proper and set standards for other languages APIs. We must not rush this out.

The correct calendars implantation is core for the Temporal API and must be implemented correctly and perfectly.

Thanks for the follow up.

@justingrant
Copy link
Collaborator

Thanks for the issue, which identified two separate bugs with the Temporal polyfill: one obvious one where the ISO calendar was being used instead of the calendar date, and a more subtle issue where some Islamic calendars (e..g islamic-umalqura) can have leap years without the leap day being added to the 12th month.

Fixes are here: #2743. We'll merge that PR once we have time to write tests to verify the fixes.

['islamic', 'islamic-umalqura', 'islamic-civil', 'islamic-rgsa', 'islamic-tbla']
  .map(calendar => Temporal.PlainDate.from({year:1445, month:1, day:1, calendar}))
  .map(d => ({
    calendar: d.calendarId,
    inLeapYear: d.inLeapYear,
    daysInYear: d.daysInYear,
    daysInMonth12: d.with({month: 12}).daysInMonth
  }))

Here's the output with the current polyfill:

0: {calendar: 'islamic', inLeapYear: false, daysInYear: 354, daysInMonth12: 29}
1: {calendar: 'islamic-umalqura', inLeapYear: true, daysInYear: 354, daysInMonth12: 30}
2: {calendar: 'islamic-civil', inLeapYear: true, daysInYear: 355, daysInMonth12: 30}
3: {calendar: 'islamic-rgsa', inLeapYear: false, daysInYear: 354, daysInMonth12: 29}
4: {calendar: 'islamic-tbla', inLeapYear: true, daysInYear: 355, daysInMonth12: 30}

Here's the output with the fix:

0: {calendar: 'islamic', inLeapYear: false, daysInYear: 354, daysInMonth12: 29}
1: {calendar: 'islamic-umalqura', inLeapYear: false, daysInYear: 354, daysInMonth12: 30}
2: {calendar: 'islamic-civil', inLeapYear: true, daysInYear: 355, daysInMonth12: 30}
3: {calendar: 'islamic-rgsa', inLeapYear: false, daysInYear: 354, daysInMonth12: 29}
4: {calendar: 'islamic-tbla', inLeapYear: true, daysInYear: 355, daysInMonth12: 30}

@MohsenAlyafei @khawarizmus @haikyuu could I get your help to verify that the "with fix" output above is what you'd expect for each calendar?

It varies with location, but it's a +- 1 day difference between any two calendars

The way ECMAScript handles this variation is by supplying 5 different Islamic calendars: 'islamic', 'islamic-umalqura', 'islamic-civil', 'islamic-rgsa', 'islamic-tbla'.

Start of day is different: the day starts at sunset!

In Temporal's initial release, calendars are concerned with dates only, not times.

As you correctly note above, some calendars (Islamic, Hebrew, and perhaps Ethiopian) consider the start of day to be at a time other than midnight and Islamic and Hebrew calendars start days at sunset. Supporting this additional complexity was beyond the scope of the initial release of Temporal, but could be accommodated in a future proposal. Feel free to file an issue here: https://github.com/js-temporal/proposal-temporal-v2

justingrant added a commit to justingrant/proposal-temporal that referenced this issue Dec 22, 2023
@justingrant
Copy link
Collaborator

FYI, there's a separate problem in the polyfill with weekOfYear and dayOfWeek properties. This is not specific to Islamic calendars but is a general question of how week numbers should be calculated for calendars that don't have a well-defined algorithm for week numbers, which AFAICT is all calendars except ISO 8601.

See #2744 for details.

Fixing this problem is out of scope to the PR mentioned above, although the Temporal champions will discuss how to handle it.

@khawarizmus
Copy link
Contributor

Thank you for the quick fix @justingrant from the example you showed it seems that the output is correct.I will go ahead and file an issue with regards supporting sunset day start as opposed to midnight.

@khawarizmus
Copy link
Contributor

@justingrant This is probably out of scope. But i have a genuine question on how can someone propose an ISO calendar? I have been thinking about designing an ISO Hijri calendar but i don't know where would I take that to become a standard.

@MohsenAlyafei
Copy link
Author

@justingrant Thanks for the quick fix. This seems to fix the 2 issues identified.
There is another issue with some islamic calendars that had been there for years in javascript and also affects Temporal but will open a separate issue for it.

BTW, is there an updated cdn link for the updated polyfill?

@MohsenAlyafei
Copy link
Author

@justingrant as you stated correctly above, there will be a difference of plus or minus 1 day between the 5 Islamic calendars.

However, it is interesting to note that the 'islamic' and 'islamic-rgsa' calendars give the same result and always agree with each other.
I have tested this in the past from the hijri years -280803 to +281510 AH by generating the reverse output from the Intl.DateTimeFormat converting islamic dates back to gregorian here: https://stackoverflow.com/questions/71222556/how-to-convert-any-of-the-5-islamic-hijri-calendars-dates-to-any-of-18-world

In fact these 2 calendars ('islamic' and 'islamic-rgsa') will break and give completely incorrect results at some hijri years. This behaviour exists before Temporal API and is also seems inherited in Temporal. When this happens these two calendars differ by 2 days from the rest of the calendars; something that should not happen !!!

Thanks again.

@khawarizmus
Copy link
Contributor

khawarizmus commented Dec 22, 2023

@MohsenAlyafei I am very unclear what the islamic calendar represents or use for it's underlying calculation even after reading about it here.

However the islamic-rgsa is the calendar based on sighting for the region of Saudi Arabia. so I am guessing for historical values it would be accurate (pulling data from a database?) then for future dates (not sighted yet) would fall back to a different algorithm which would create this inconsistent behaviour and drift.

I couldn't find more details on how the islamic-rgsa works but i don't see a way for a sighting calendar to foresee future dates except to fall back on calculations again. unless sighting here means something else.

I am also curious to understand why these two calendars always yield the same results

@justingrant
Copy link
Collaborator

i have a genuine question on how can someone propose an ISO calendar? I have been thinking about designing an ISO Hijri calendar but i don't know where would I take that to become a standard.

There are many different standards bodies, so I'm not sure where best to recommend. I think a good first step could be to ask by filing an issue in the ECMA-402 repo (https://github.com/tc39/ecma402), which is the group responsible for internationalization in ECMAScript.

Actual calendar calculations in ECMAScript engines (and Java and...) are performed by an underlying library called ICU. Any fixes to changes to actual calendar calculations would happen in that library. But filing an issue over in ECMA 402 is a good first step, unless you want to write your own C++ or Java app that calls ICU directly. In that case you can consider filing issues in ICU's JIRA with repro steps and recommended behavior.

I'm sorry I don't have a clearer recommendation for you. Temporal is just a wrapper around lower-level functionality provided by ECMAScript engines, so we don't actually have any control over what calendars are supported.

In fact these 2 calendars ('islamic' and 'islamic-rgsa') will break and give completely incorrect results at some hijri years. This behaviour exists before Temporal API and is also seems inherited in Temporal. When this happens these two calendars differ by 2 days from the rest of the calendars; something that should not happen !!!

If you have a bug with clear repro steps, definitely file an issue in ECMA-402 and hopefully you'll get advice there about what to do next.

There is another issue with some islamic calendars that had been there for years in javascript and also affects Temporal but will open a separate issue for it.

For issues with the underlying calendar that are not related to Temporal, a good place to file them is to start with the ECMA 402 repo.

BTW, is there an updated cdn link for the updated polyfill?

The polyfill in this repo is intended only for testing the Temporal API, not for production use. Production polyfills are under construction. You can see links to them here. I'm a maintainer of one of those libraries (@js-temporal/polyfill) and we're a few months behind the latest spec changes to Temporal, but hopefully we'll be able to port changes over soon. I'm not sure about the status of the other production polyfill, but they may be further ahead.

Note that soon (hopefully some time in 2024) you won't need a polyfill at all, because JS engines are building Temporal natively.

Good luck, and thanks again for your feedback and for helping us find bugs.

@khawarizmus
Copy link
Contributor

I'm sorry I don't have a clearer recommendation for you.

Thanks a lot for taking the time to reply. I appreciate it. I think it's more then helpful

justingrant added a commit to justingrant/proposal-temporal that referenced this issue Jan 12, 2024
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 a pull request may close this issue.

4 participants