Update duration rounding to new algorithms #65

nekevss · 2024-06-30T20:56:38Z

~~Currently a WIP.~~

This closes #19 outside of anything related to ZonedDateTime, which I'd prefer to consider separately due to the reliance on time zones.

This PR updates duration rounding to the new duration rounding algorithms.

As it's a bit of a larger PR, I thought I would create a draft in case anyone wants to take a look early.

Overall, this draft is fairly close to complete, but there are still at least a couple bugs around the actually rounding that I'm working through.

nekevss · 2024-07-02T00:12:12Z

This PR should definitely wait until after #66 is merged so it can be rebased to remove the custom traits, but it is essentially ready for review outside of that impending rebase

There are a couple things to note:

This implements the new rounding methods under a round_v2 and doesn't remove the previous one just yet. It can be removed if everyone else thinks it should be in this PR, but I was sort of thinking about lagging on removing the previous methods while the rest is tested. Hopefully, it also makes the diff just a little bit easier to read.
Any rounding around ZonedDateTime is not included in this, but it's hard to implement and test that without proper time zone support, so if everyone else is in agreement I'd prefer to file that under a new issue.
We should probably file an issue for more unit tests around the internal nudge algorithms and normalization records.

jedel1043

Looks great! I have some suggestions on some APIs that we can improve.

src/components/duration.rs

src/components/duration/normalized.rs

src/utils.rs

…ring

src/options.rs

src/components/duration.rs

jedel1043 · 2024-07-06T00:08:10Z

src/options.rs

+    pub(crate) fn from_diff_settings(
+        options: DifferenceSettings,
+        operation: DifferenceOperation,
+        fallback_largest: TemporalUnit,
+        fallback_smallest: TemporalUnit,
+    ) -> TemporalResult<(f64, Self)> {


Thought: Maybe we could return NonZeroDurationSign here? It would be more consistent to use a single type for the signs instead of having a mix of i8, f64, Sign and NonZeroDurationSign everywhere.

Updated to a Sign value.

jedel1043 · 2024-07-06T00:22:51Z

src/components/duration/time.rs

@@ -365,6 +370,69 @@ impl TimeDuration {
 // ==== TimeDuration method impls ====

 impl TimeDuration {
+    // TODO: Maybe move to `NormalizedTimeDuration`
+    pub fn round_v2(


Should this be replacing the original round? I think we decided to remove the original rounding functions instead of having a v2 for them.

I went to remove it, but there's still a dependency on the original round. I was going to follow up on this in a different PR.

EDIT: Went through and cleaned it up.

jedel1043 · 2024-07-06T00:24:27Z

src/components/time.rs

@@ -30,9 +32,28 @@ impl Time {
        self.iso.is_valid()
    }

+    /// Specification equivalent to `AddTime`
+    pub(crate) fn add_norm(&self, norm: NormalizedTimeDuration) -> (i32, Self) {


Nit: probably use the full name of add_normalized_time_duration. add_norm is a bit more confusing because it has a different meaning mathematically.

jedel1043 · 2024-07-06T00:25:56Z

src/options.rs

+#[derive(Debug, Clone, Copy)]
+pub(crate) struct ResolvedRoundingOptions {
+    pub(crate) largest_unit: TemporalUnit,
+    pub(crate) smallest_unit: TemporalUnit,
+    pub(crate) increment: RoundingIncrement,
+    pub(crate) rounding_mode: TemporalRoundingMode,
+}


Praise: Loving this new API! I think it really makes sense to have an options struct that resolves all the Options in RoundingOptions, instead of having to unwrap everywhere.

jedel1043 · 2024-07-06T00:42:18Z

src/components/duration.rs

+#[repr(i8)]
+#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)]
+pub enum DurationSign {
+    Positive = 1,
+    Zero = 0,
+    Negative = -1,
+}
+
+impl From<Ordering> for DurationSign {
+    fn from(value: Ordering) -> Self {
+        match value {
+            Ordering::Greater => Self::Positive,
+            Ordering::Equal => Self::Zero,
+            Ordering::Less => Self::Negative,
+        }
+    }
+}
+
+#[repr(i8)]
+#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)]
+pub(crate) enum NonZeroDurationSign {
+    Positive = 1,
+    Negative = -1,
+}
+
+impl From<DurationSign> for NonZeroDurationSign {
+    fn from(value: DurationSign) -> Self {
+        if matches!(value, DurationSign::Negative) {
+            return Self::Negative;
+        }
+        Self::Positive
+    }
+}
+


After seeing how NonZeroDurationSign basically optimizes the same as DurationSign, I don't think it's worth it anymore. Having just DurationSign should be okay, IMO.

I'm going back and forth with DurationSign vs. Sign. 😕 Updated to Sign this time around.

jedel1043 · 2024-07-06T00:46:34Z

src/components/duration.rs

+impl From<Ordering> for DurationSign {
+    fn from(value: Ordering) -> Self {
+        match value {
+            Ordering::Greater => Self::Positive,
+            Ordering::Equal => Self::Zero,
+            Ordering::Less => Self::Negative,
+        }
+    }
+}


Hmmm, I don't know if it makes much sense to have a conversion from these enums. Ordering is a completely different concept than DurationSign, and things like DurationSign::from(zero.cmp(&target)) don't make much sense if you think about it.

Yeah, it's a bit awkward. I wasn't super in love with it. Maybe just a From<i8>

jedel1043 · 2024-07-06T02:37:48Z

src/components/duration/normalized.rs

+    pub(crate) fn norm(&self) -> NormalizedTimeDuration {
+        self.norm
+    }


Same suggestion of using the full name.

jedel1043 · 2024-07-06T02:46:12Z

src/lib.rs

+    pub(crate) fn as_non_zero(&self) -> Self {
+        if matches!(self, Self::Zero) {
+            return Self::Positive;
+        }
+        *self
+    }


If this is to multiply a duration with the sign, what about having an as_sign_multiplier function instead that returns 1i8 for zero and positive, and -1i8 for negative?

It is. It's specifically a nonzero variant in these rounding methods vs. the regular multiplication that occurs elsewhere. I'm fine with as_sign_multiplier though. Better than anything I'm coming up with in this instance.

jedel1043

Really impressive work! Pretty much perfect IMO :)

jasonwilliams

LGTM!
As @jedel said this looks great, big improvement

nekevss marked this pull request as ready for review July 1, 2024 23:59

nekevss changed the title ~~WIP: Update duration rounding to new algorithms~~ Update duration rounding to new algorithms Jul 1, 2024

nekevss requested review from jasonwilliams and jedel1043 July 1, 2024 23:59

jedel1043 reviewed Jul 3, 2024

View reviewed changes

nekevss added 5 commits July 3, 2024 17:06

Progress implementing the new methods

af6cc59

Complete progress on initial working draft

7e92607

Move nudge algorithms to NormalizedDurationRecord

b3f09c9

Post-rebase cargo fmt and small fix in README

cb6c41a

Remove previous round implementation and adjust fix tests

e34d24b

nekevss force-pushed the update-duration-rounding branch from 009faf9 to e34d24b Compare July 3, 2024 23:57

nekevss added 2 commits July 4, 2024 10:02

Refactor options object for rounding and diffing

21170bf

Refactor duration sign handling, add DurationSign, remove NonZeroOrde…

9b655c3

…ring

nekevss requested a review from jedel1043 July 4, 2024 22:42

jedel1043 requested changes Jul 6, 2024

View reviewed changes

jedel1043 reviewed Jul 6, 2024

View reviewed changes

Apply review feedback

5238808

jedel1043 reviewed Jul 6, 2024

View reviewed changes

nekevss added 2 commits July 5, 2024 22:58

review feedback on asSignMulitplier and normalizedTimeDuration methods

5175812

Remove round method from DateDuration and unused associated methods

515a2a4

jedel1043 approved these changes Jul 6, 2024

View reviewed changes

jasonwilliams approved these changes Jul 6, 2024

View reviewed changes

nekevss merged commit c999bb3 into main Jul 6, 2024
5 checks passed

jedel1043 deleted the update-duration-rounding branch July 6, 2024 17:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update duration rounding to new algorithms #65

Update duration rounding to new algorithms #65

nekevss commented Jun 30, 2024 •

edited

Loading

nekevss commented Jul 2, 2024

jedel1043 left a comment

jedel1043 Jul 6, 2024

nekevss Jul 6, 2024

jedel1043 Jul 6, 2024

nekevss Jul 6, 2024 •

edited

Loading

jedel1043 Jul 6, 2024

jedel1043 Jul 6, 2024

jedel1043 Jul 6, 2024 •

edited

Loading

nekevss Jul 6, 2024

jedel1043 Jul 6, 2024

nekevss Jul 6, 2024

jedel1043 Jul 6, 2024

jedel1043 Jul 6, 2024

nekevss Jul 6, 2024

jedel1043 left a comment

jasonwilliams left a comment

Update duration rounding to new algorithms #65

Update duration rounding to new algorithms #65

Conversation

nekevss commented Jun 30, 2024 • edited Loading

nekevss commented Jul 2, 2024

jedel1043 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nekevss Jul 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jedel1043 Jul 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jedel1043 left a comment

Choose a reason for hiding this comment

jasonwilliams left a comment

Choose a reason for hiding this comment

nekevss commented Jun 30, 2024 •

edited

Loading

nekevss Jul 6, 2024 •

edited

Loading

jedel1043 Jul 6, 2024 •

edited

Loading