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

[Maybe breaking change] Fix day of year computation #273

Merged
merged 8 commits into from
Jan 3, 2024

Conversation

ChristopherRabotin
Copy link
Member

The day of year computation was broken: first, it didn't work well when the time scale reference epoch was not J1900 (cf #272 ), and moreover, it started counting the days at zero, when it should have one (to meet the GPS standard and the Python behavior).

@@ -1008,6 +1008,7 @@ impl Add for Duration {
/// ## Examples
/// + `Duration { centuries: 0, nanoseconds: 1 }` is a positive duration of zero centuries and one nanosecond.
/// + `Duration { centuries: -1, nanoseconds: 1 }` is a negative duration representing "one century before zero minus one nanosecond"
#[allow(clippy::absurd_extreme_comparisons)]
Copy link
Member Author

Choose a reason for hiding this comment

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

Prevent what clippy said was useless but actually required.

@@ -90,7 +90,7 @@ const MAX_TOKENS: usize = 16;
/// assert_eq!(fmt, consts::ISO8601_ORDINAL);
///
/// let fmt_iso_ord = Formatter::new(bday, consts::ISO8601_ORDINAL);
/// assert_eq!(format!("{fmt_iso_ord}"), "2000-059");
/// assert_eq!(format!("{fmt_iso_ord}"), "2000-060");
Copy link
Member Author

Choose a reason for hiding this comment

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

Behavior change introduced by this PR.

let e = Epoch::from_format_str("2023-117T12:55:26", "%Y-%jT%H:%M:%S").unwrap();

assert_eq!(format!("{e}"), "2023-04-28T12:55:26 UTC");
assert_eq!(format!("{e}"), "2023-04-27T12:55:26 UTC");
Copy link
Member Author

Choose a reason for hiding this comment

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

Now matches Python as per the comment.

Copy link

codecov bot commented Dec 30, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f37268b) 80.77% compared to head (401b9c9) 81.05%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #273      +/-   ##
==========================================
+ Coverage   80.77%   81.05%   +0.27%     
==========================================
  Files          16       16              
  Lines        3776     3784       +8     
==========================================
+ Hits         3050     3067      +17     
+ Misses        726      717       -9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ChristopherRabotin
Copy link
Member Author

This should be tagged at 3.9.0 because of that behavior change.

@gwbres gwbres mentioned this pull request Dec 30, 2023
@gwbres
Copy link
Collaborator

gwbres commented Dec 31, 2023

I cannot push contributions to this PR since I did not initiate it.
Though I did check that the test you wrote (for GPST + UTC) does pass in other timescales.
I may provide these additional tests in a separate PR.

@ChristopherRabotin
Copy link
Member Author

ChristopherRabotin commented Dec 31, 2023 via email

@gwbres
Copy link
Collaborator

gwbres commented Dec 31, 2023

Ok,

here's a test that does pass

#[test]
fn regression_test_gh_272() {
    use core::str::FromStr;

    let epoch = Epoch::from_str("2021-12-21T00:00:00 GPST").unwrap();

    let (years, day_of_year) = epoch.year_days_of_year();

    assert!(dbg!(day_of_year) < DAYS_PER_YEAR);
    assert!(day_of_year > 0.0);
    assert_eq!(day_of_year, 355.0);

    assert_eq!(years, 2021);

    // Check that we start counting the days at one, in all timescales.
    for ts in ["TAI", "UTC", "GPST", "GST", "BDT"] {
        let epoch = Epoch::from_str(&format!("2021-12-31T00:00:00 {}", ts)).unwrap();
        let (years, day_of_year) = epoch.year_days_of_year();
        assert_eq!(years, 2021);
        assert_eq!(day_of_year, 365.0);

        let epoch = Epoch::from_str(&format!("2020-12-31T00:00:00 {}", ts)).unwrap();
        let (years, day_of_year) = epoch.year_days_of_year();
        assert_eq!(years, 2020);
        // 366 days in 2020, leap year.
        assert_eq!(day_of_year, 366.0);

        let epoch = Epoch::from_str(&format!("2021-01-01T00:00:00 {}", ts)).unwrap();
        let (years, day_of_year) = epoch.year_days_of_year();
        assert_eq!(years, 2021);
        assert_eq!(day_of_year, 1.0);
    }
}

@ChristopherRabotin
Copy link
Member Author

I added those. The only difference is that I avoided the format!() instruction because it's only available with the std feature.

@ChristopherRabotin
Copy link
Member Author

@gwbres Let me know if this looks good to merge and would fix your usecase.

@gwbres
Copy link
Collaborator

gwbres commented Jan 3, 2024

Hello, yes it alll looks fine to me

@ChristopherRabotin ChristopherRabotin merged commit 1fe2789 into master Jan 3, 2024
31 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.

2 participants