tc39 / proposal-temporal

Provides standard objects and functions for working with dates and times.
https://tc39.es/proposal-temporal/docs/
Other
3.33k stars 150 forks source link

Islamic Calendars incorrect dayOfYear #2740

Closed MohsenAlyafei closed 9 months ago

MohsenAlyafei commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

we have no ISO Hijri calendar

Here are some special differences from Gregorian 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 commented 10 months ago

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 commented 10 months ago

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

MohsenAlyafei commented 10 months ago

@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 commented 10 months ago

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: https://github.com/tc39/proposal-temporal/pull/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 commented 10 months ago

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 https://github.com/tc39/proposal-temporal/issues/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 commented 10 months ago

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 commented 10 months ago

@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 commented 10 months ago

@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 commented 10 months ago

@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 commented 10 months ago

@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 commented 10 months ago

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 commented 10 months ago

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