Implement Neri-Schneider calculations #147
Conversation
|
@nekevss Thanks for your interest in our work and for this implementation. Lorenz and I are always glad to see our algorithms to be implemented by others in different computer systems. I took the liberty to inspect the assembly generated by your implementation through compiler explorer. Here is the link. It looks quite nice already but it can be improved a bit further. In the paper we define a constant IMHO, it's better to define |
nekevss
force-pushed
the
impl-neri-schneider
branch
from
f625541 to
bae8b93
Compare
Updating per review from @cassioneri to optimize instructions Co-authored-by: Cassio Neri <[email protected]>
src/neri_schneider.rs
Outdated
| //! | September 14, 275,760 | 100_719_469 | 200,065,429 | | ||
| //! | ||
| //! However, this shift has also been implemented by Cassio Neri, who | ||
| //! recommends using a [shift of 3670][] which places the Epoch in the |
src/neri_schneider.rs
Outdated
| // NOTE: Temporal must support the year range [-271_821, 275_760] | ||
| // | ||
| // This means the epoch day range must be epoch_days.abs() <= 100_000_001 | ||
| // | ||
| // Neri-Schneider mention shifting for a range of 32_767, so the shift | ||
| // will need to be much greater. | ||
| // | ||
| // (-271_821 / 400).ciel() = s // 680 | ||
| // |
There was a problem hiding this comment.
Maybe integrate this into the documentation above? Seems important enough to specify the complete range that temporal_rs supports.
src/neri_schneider.rs
Outdated
| const TWO_POWER_THIRTY_NINE: u64 = 549_755_813_888; // 2^39 constant | ||
| const TWO_POWER_THIRTY_TWO: u64 = 4_294_967_296; // 2^32 constant | ||
| const TWO_POWER_SIXTEEN: u32 = 65_536; // 2^16 constant | ||
| const DAYS_IN_GREGORIAN_CYCLE: i32 = DAYS_IN_A_400Y_CYCLE as i32; |
There was a problem hiding this comment.
I'd just use the same constant and convert to i32 explicitly. Seems simpler than having two separate but equal constants.
src/neri_schneider.rs
Outdated
|
|
||
| /// Calculate Rata Die value from gregorian | ||
| pub fn epoch_days_from_gregorian_date(year: i32, month: i32, day: i32) -> i32 { | ||
| let shift = SHIFT_CONSTANT * DAYS_IN_GREGORIAN_CYCLE + EPOCH_COMPUTATIONAL_RATA_DIE; |
There was a problem hiding this comment.
Make this a const? It only depends on const values, after all.
src/neri_schneider.rs
Outdated
| const SHIFT_CONSTANT: i32 = 3670; | ||
|
|
||
| /// Calculate Rata Die value from gregorian | ||
| pub fn epoch_days_from_gregorian_date(year: i32, month: i32, day: i32) -> i32 { |
There was a problem hiding this comment.
I'm a bit confused by the types here. If I understood the code correctly, it relies on month and day being positive and greater than 1, so why do we want to use signed integers in this case?
There was a problem hiding this comment.
Ooooh, good point. Actually, I think this might have been a bug on the implementation side connected to IsoDate::balance
src/neri_schneider.rs
Outdated
There was a problem hiding this comment.
I'm a bit skeptical about exposing this as a public module. I would expect this to be just internal details of our utils module.
There was a problem hiding this comment.
This is still exposed as a public (crate) module. I think the best option here is to have neri_schneider be a submodule of utils, then just avoid exposing implementation details to the rest of the library.
There was a problem hiding this comment.
Ah, I misread that for some reason. Making it a submodule of utils sounds fine to me.
There was a problem hiding this comment.
Really nice implementation! There are still some details that we could definitely improve — explain what "rata die", "computational calendar", "first equation", etc. mean, maybe change some names to be more descriptive — but I think this is good enough that we can merge it and then iteratively improve on it.
This PR changes the current date calculations to one's layed out by Neri-Schneider's Euclidean affine functions paper.
There is still some documentation that needs to be completed before this is ready to merge. I was also hoping to double check the assembly but I'm sort of viewing that as part of the documentation. In the meantime, I'm posting this as a draft for feedback.