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

Add override table for Persian calendar #4796

Merged
merged 4 commits into from
Apr 13, 2024
Merged

Add override table for Persian calendar #4796

merged 4 commits into from
Apr 13, 2024

Conversation

roozbehp
Copy link
Contributor

Fixes #4713 until about 3000 AP (~3621 CE).

Fixes #4713 until about 3000 AP (~3621 CE).
@roozbehp roozbehp requested review from a team, Manishearth and sffc as code owners April 11, 2024 07:21
@roozbehp
Copy link
Contributor Author

roozbehp commented Apr 11, 2024

Hi! Sorry for the trouble. I don't understand why some of the github tests fail. cargo test --all-features works fine on my local machine. My guess is that I'm not supposed to use either lazy_static or HashSet (or both). Since I'm quite new to Rust and ICU4X, I would appreciate pointers to what I should use instead.

@Manishearth
Copy link
Member

Yeah we target embedded environments so a bunch of things that require the rust standard library are not available. I'll try and fix this using a binary search table or something.

@Manishearth
Copy link
Member

Pushed a patch to use a binary search

@roozbehp
Copy link
Contributor Author

Thank you very much for the fixes. LGTM.

Comment on lines +24 to +28
1502, 1601, 1634, 1667, 1700, 1733, 1766, 1799, 1832, 1865, 1898, 1931, 1964, 1997, 2030, 2059,
2063, 2096, 2129, 2158, 2162, 2191, 2195, 2224, 2228, 2257, 2261, 2290, 2294, 2323, 2327, 2356,
2360, 2389, 2393, 2422, 2426, 2455, 2459, 2488, 2492, 2521, 2525, 2554, 2558, 2587, 2591, 2620,
2624, 2653, 2657, 2686, 2690, 2719, 2723, 2748, 2752, 2756, 2781, 2785, 2789, 2818, 2822, 2847,
2851, 2855, 2880, 2884, 2888, 2913, 2917, 2921, 2946, 2950, 2954, 2979, 2983, 2987,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Observation: it's hard not to notice that more leap year overrides are needed the farther into the future we go.

Century # Corrections
1200 AP 0
1300 AP 0
1400 AP 0
1500 AP 1
1600 AP 3
1700 AP 4
1800 AP 3
1900 AP 3
2000 AP 4
2100 AP 5
2200 AP 6
2300 AP 6
2400 AP 6
2500 AP 6
2600 AP 6
2700 AP 8
2800 AP 8
2900 AP 9

Is there some pattern to the years that need correcting? Can they be fixed by improving the approximation somehow?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there some pattern to the years that need correcting? Can they be fixed by improving the approximation somehow?

I looked into it, but couldn't find a good exact pattern that would also lend itself to conversion to and from “fixed” dates.

Generally, the more into the future you go, the more chaotic it becomes. Is it because of actual astronomical phenomena? Or is it because of the peculiarities of the approximations made in the astronomical calculations code? I have no idea.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pretty characteristic of doing a local periodic approximation of complex phenomena. Over time the errors start growing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pretty characteristic of doing a local periodic approximation of complex phenomena. Over time the errors start growing.

Yeah, my question was whether we should consider adding approximations over other periods instead of "patching" the approximation.

@Manishearth Manishearth merged commit a01a311 into unicode-org:main Apr 13, 2024
30 checks passed
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 this pull request may close these issues.

Persian calendar conversion uses wrong algorithm
3 participants