Change `AstronomicalSimulation` #7342

robertbastian · 2025-12-23T16:01:35Z

No description provided.

sffc

I think this direction is consistent with #7340, since it takes advantage of the bullet point "We decide to tweak the precise astronomical simulation used" and keeps the type AstronomicalSimulation as representing an astronomical simulation, just a different one than we had been shipping before.

components/calendar/src/any_calendar.rs

sffc · 2025-12-23T17:17:06Z

components/calendar/src/any_calendar.rs

                Ok(AnyCalendarKind::HijriTabularTypeIIFriday)
            }
-            Hijri(Some(HijriCalendarAlgorithm::Rgsa)) => Ok(AnyCalendarKind::HijriSimulatedMecca),
+            Hijri(Some(HijriCalendarAlgorithm::Rgsa)) => Err(()),


We don't return Err elsewhere in this fn; what is the impact?

yes we do, for -u-ca-islamic

Thank you for removing this mapping.

I think this creates a new code path. -u-ca-islamic doesn't make it into this impl; it is remapped in resolved_algorithm() above. This new case means that -u-ca-islamic-rgsa maps unconditionally to Gregorian due to the unwrap_or(Self::Gregorian) in AnyCalendarKind::new. It should instead map to the locale default.

should it go to the locale default, or should it behave like -u-ca-islamic which goes to the locale default Hijri?

You've now specifically documented, on resolved_algorithm,

/// This is guaranteed to return a [`CalendarAlgorithm`] that successfully converts into an /// [`AnyCalendarKind`] (i.e. not `-u-ca-islamic` or `-u-ca-islamic-rgsa`).

I feel like islamic-rgsa should have the same behavior as islamic-zzz.

components/calendar/src/cal/hijri.rs

Co-authored-by: Manish Goregaokar <[email protected]>

Manishearth

In favor of the technical implementation here.

Exact future for calendars still seems to be in flux, I am personally in favor of this being a component of most paths forward (potentially paired with some deprecation), but see #7320

hsivonen

Thank you. Removing the rgsa mapping and putting the Reingold simulation behind a flag resolve my main concerns, though I still think we shouldn't have this level of attachment to keeping the Reingold simulation.

Since AstronomicalSimulation exist for semver and duplicates to a Umm al-Qura, it seems to me AstronomicalSimulation should be marked deprecated.

hsivonen · 2026-01-13T05:46:22Z

components/calendar/src/cal/hijri.rs

+/// - We decide to expand or reduce the range where we are using the astronomical simulation.
+#[derive(Copy, Clone, Debug, PartialEq, Eq)]
+#[non_exhaustive]
+pub struct AstronomicalSimulation;


Why isn't this deprecated if this duplicates Umm al-Qura behavior and exists for semver?

I'm trying to do this in as small chunks as possible to make some progress

hsivonen · 2026-01-13T05:47:08Z

components/calendar/src/any_calendar.rs

                Ok(AnyCalendarKind::HijriTabularTypeIIFriday)
            }
-            Hijri(Some(HijriCalendarAlgorithm::Rgsa)) => Ok(AnyCalendarKind::HijriSimulatedMecca),
+            Hijri(Some(HijriCalendarAlgorithm::Rgsa)) => Err(()),


Thank you for removing this mapping.

components/calendar/src/cal/hijri.rs

sffc

This is a technical and docs review.

components/calendar/src/cal/hijri.rs

components/calendar/src/any_calendar.rs

sffc · 2026-01-13T22:12:56Z

components/calendar/src/any_calendar.rs

                Ok(AnyCalendarKind::HijriTabularTypeIIFriday)
            }
-            Hijri(Some(HijriCalendarAlgorithm::Rgsa)) => Ok(AnyCalendarKind::HijriSimulatedMecca),
+            Hijri(Some(HijriCalendarAlgorithm::Rgsa)) => Err(()),


I think this creates a new code path. -u-ca-islamic doesn't make it into this impl; it is remapped in resolved_algorithm() above. This new case means that -u-ca-islamic-rgsa maps unconditionally to Gregorian due to the unwrap_or(Self::Gregorian) in AnyCalendarKind::new. It should instead map to the locale default.

hsivonen

I meant to choose the Approve option earlier.

sffc · 2026-01-15T19:59:13Z

Consensus: As agreed in #7422, this can be handled by migration notes; we don't need to retain the ReingoldSimulation type.

sffc · 2026-01-16T17:31:10Z

components/calendar/src/any_calendar.rs

                Ok(AnyCalendarKind::HijriTabularTypeIIFriday)
            }
-            Hijri(Some(HijriCalendarAlgorithm::Rgsa)) => Ok(AnyCalendarKind::HijriSimulatedMecca),
+            Hijri(Some(HijriCalendarAlgorithm::Rgsa)) => Err(()),


You've now specifically documented, on resolved_algorithm,

/// This is guaranteed to return a [`CalendarAlgorithm`] that successfully converts into an /// [`AnyCalendarKind`] (i.e. not `-u-ca-islamic` or `-u-ca-islamic-rgsa`).

I feel like islamic-rgsa should have the same behavior as islamic-zzz.

sffc · 2026-01-16T17:33:10Z

components/calendar/tests/reingold.rs

+///
+/// [^1]: See [calendrical_calculations::islamic::observational_islamic_from_fixed]
+#[derive(Debug, Clone, Copy)]
+pub struct ReingoldSimulation;


Praise: good to retain this code even if outside of src. Is tests the right place, or should it be in examples?

I don't want to advertise this

sffc · 2026-01-17T08:07:25Z

components/calendar/src/any_calendar.rs

+    /// The kind of a [`HijriSimulated`] calendar
    ///
-    /// This corresponds to the `"islamic-rgsa"` [CLDR calendar](https://unicode.org/reports/tr35/#UnicodeCalendarIdentifier).
+    /// This does not correspond to a CLDR calendar.


Issue: Well, actually it does correspond to a CLDR calendar, just not one that ICU4X chooses to implement. The previous language is still correct.

sffc · 2026-01-17T08:09:01Z

components/calendar/src/any_calendar.rs

    /// assert_eq!(CalendarPreferences::from(&locale!("und-AF")).resolved_algorithm(), CalendarAlgorithm::Persian);
    /// assert_eq!(CalendarPreferences::from(&locale!("und-US-u-rg-thxxxx")).resolved_algorithm(), CalendarAlgorithm::Buddhist);
+    /// assert_eq!(
+    ///     CalendarPreferences::from(&locale!("und-US-u-ca-islamic")).resolved_algorithm(),


Nit: We should have a test for what happens with islamic-zzz, islamic-rgsa, and buddhist-zzz.

robertbastian force-pushed the simulated branch 2 times, most recently from 99cd652 to 5ea69c0 Compare December 23, 2025 17:13

sffc reviewed Dec 23, 2025

View reviewed changes

robertbastian added 2 commits December 30, 2025 15:52

rgsa

38499a5

reingold

5652bd5

robertbastian force-pushed the simulated branch from 1083fa9 to 5652bd5 Compare December 30, 2025 14:53

robertbastian marked this pull request as ready for review December 30, 2025 14:53

robertbastian requested review from Manishearth and zbraniecki as code owners December 30, 2025 14:53

robertbastian requested a review from sffc December 30, 2025 14:53

stabilisation todo

cfb5f9d

Manishearth reviewed Dec 30, 2025

View reviewed changes

components/calendar/src/cal/hijri.rs Outdated Show resolved Hide resolved

Update components/calendar/src/cal/hijri.rs

e0bde8b

Co-authored-by: Manish Goregaokar <[email protected]>

robertbastian requested a review from Manishearth January 6, 2026 14:01

renamed data module

34ecb04

Manishearth approved these changes Jan 6, 2026

View reviewed changes

hsivonen reviewed Jan 13, 2026

View reviewed changes

sffc reviewed Jan 13, 2026

View reviewed changes

Manishearth mentioned this pull request Jan 13, 2026

ICU4X 2.2 headliner: New and Improved icu_calendar #7422

Open

hsivonen approved these changes Jan 15, 2026

View reviewed changes

move ReingoldSimulation to a test

d5f29ea

robertbastian changed the title ~~AstronomicalSimulation -> ReingoldSimulation~~ Change AstronomicalSimulation Jan 16, 2026

robertbastian added 2 commits January 16, 2026 11:45

Merge remote-tracking branch 'upstream/main' into simulated

065ceb2

docs

e7feeab

sffc reviewed Jan 16, 2026

View reviewed changes

resolve

467e6ad

robertbastian requested a review from sffc January 16, 2026 21:48

sffc reviewed Jan 17, 2026

View reviewed changes

Change AstronomicalSimulation #7342

Are you sure you want to change the base?

Change AstronomicalSimulation #7342

Uh oh!

Conversation

robertbastian commented Dec 23, 2025

Uh oh!

sffc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Manishearth left a comment

Choose a reason for hiding this comment

Uh oh!

hsivonen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sffc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hsivonen left a comment

Choose a reason for hiding this comment

Uh oh!

sffc commented Jan 15, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Change `AstronomicalSimulation` #7342

Change `AstronomicalSimulation` #7342