Fix conversion to Gregorian #303
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…minal cases. I prefer the new approach (less cycles) so I'll work to fix that until I fix the other tests
There's an off by one error in the number of days but I don't know why
Also remove commented code
| None => { | ||
| return Err(EpochError::Duration { | ||
| source: DurationError::Overflow, | ||
| }) | ||
| } | ||
| Some(days) => Unit::Day * i64::from(days), | ||
| }, | ||
| } - time_scale.gregorian_epoch_offset(); |
There was a problem hiding this comment.
Moved to the end of the function to make it easier to write the reverse algorithm.
| @@ -269,45 +251,43 @@ impl Epoch { | |||
| source: DurationError::Underflow, | |||
| }) | |||
| } | |||
| Some(years_since_ref) => match years_since_ref.checked_mul(365) { | |||
| Some(years_since_ref) => match years_since_ref.checked_mul(DAYS_PER_YEAR_NLD as i32) { | |||
There was a problem hiding this comment.
Replace magic number with the constant.
| duration_wrt_ref += Unit::Day; | ||
| } | ||
| } | ||
| // Remove ref hours from duration to correct for the time scale not starting at midnight | ||
| // duration_wrt_ref -= Unit::Hour * time_scale.ref_hour() as i64; |
There was a problem hiding this comment.
This had been replaced with the time_scale.gregorian_offset()
| @@ -1370,9 +1370,14 @@ fn div_rem_f64(me: f64, rhs: f64) -> (i32, f64) { | |||
| fn div_euclid_f64(lhs: f64, rhs: f64) -> f64 { | |||
| let q = (lhs / rhs).trunc(); | |||
| if lhs % rhs < 0.0 { | |||
| return if rhs > 0.0 { q - 1.0 } else { q + 1.0 }; | |||
| if rhs > 0.0 { | |||
There was a problem hiding this comment.
Exact same code, just more idiomatic Rust.
ChristopherRabotin
changed the title
Closed
| @@ -692,7 +690,15 @@ impl fmt::Display for Duration { | |||
| } | |||
|
|
|||
| let values = [days, hours, minutes, seconds, milli, us, nano]; | |||
| let units = ["days", "h", "min", "s", "ms", "μs", "ns"]; | |||
| let units = [ | |||
| if days > 1 { "days" } else { "day" }, | |||
There was a problem hiding this comment.
Super minor but it was driving me crazy that a duration of one day was printed in plural form.
This was referenced Jun 9, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue #261 made it clear that there were bugs in the conversion from a duration in a given time scale into its Gregorian representation. This PR solves this by rewriting that algorithm and extending the tests to include many reciprocal tests.
One of the complications I had was correcting for the leap years when converting a duration to its Gregorian representation. Specifically, the number of leap days in the "current year" is handled by the array of cumulative number of days. But if there were more leap years between 1900 and the current year was greater than the number of days left in a given year, then the modulo operation would incorrectly say that we were one year later (e.g. 1988-12-25 was computed as 1989-01-...). The number of days in the year was trivially fixed, but that fix failed if the previous year (e.g. 1988) was a leap year, since the code would incorrectly subtract one day to that year when that fix was in fact handled by the array of cumulative number of days.