Skip to content

Conversation

adrian-kong
Copy link
Contributor

@adrian-kong adrian-kong commented Jul 12, 2022

Add a total_cmp method similar to the f64::total_cmp introduced in Rust 1.62 -- this should simplify operations that require sorting GpsTime values (and other related operations that require a total order).

@adrian-kong adrian-kong requested a review from notoriaga July 12, 2022 04:41
@@ -299,6 +300,10 @@ impl GpsTime {
assert!(self >= GLO_TIME_START);
GloTime(unsafe { swiftnav_sys::gps2glo(self.c_ptr(), std::ptr::null()) })
}

pub fn total_cmp(&self, other: &GpsTime) -> Ordering {
self.tow().total_cmp(&other.tow())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't we need to also compare the week number in certain circumstances?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I'm thinking this might need to be something like

impl GpsTime {
    pub fn to_seconds(&self) -> f64 {
        (self.wn() * WEEK_SECS) + self.tow()
    }
    pub fn total_cmp(&self, other: &GpsTime) -> Ordering { 
        self.to_seconds().total_cmp(other.to_seconds())   
    }
}

But also I'm wondering if we can just impl Ord for GpsTime. Internally we are using an f64/double which doesn't implement ord. But, I think GpsTimes are guaranteed to not have tow be Nan/Inf though, which is why rust doesn't have f64: Ord. Although that might just be at construction. I think we might be able to end up with a NaN tow via addition/subtraction? Not sure if that's actually possible though. We do allow tow to be 0, which could cause NaNs via division, but that seems irrelevant because we aren't dividing anywhere. We also aren't using log/sqrt which are the other big NaN culprits.

The PartialOrd impl seems to always return Some, which makes me feel like we do in fact have a total ordering. So maybe our Ord could just be

impl Ord for GpsTime {
    fn cmp(&self, other: &Self) -> std::cmp::Ordering {
        self.partial_cmp(other).unwrap()
    }
}

I'm a little hesitant to do that though, because messing up your Ord/PartialOrd impls can cause some nasty bugs. But it would sure be convenient. Maybe we could add a bunch of debug_asserts throughout checking for .is_finite() + expand on the testing we due for ordering?

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like we might be ok? https://en.wikipedia.org/wiki/NaN#Operations_generating_NaN

I think we'd need to add assert!(soln_freq.is_finite()); in round_to_epoch/floor_to_epoch. Everything else seems like addition/subtraction with two non-NaN values

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've attempted to make it very difficult to create a GpsTime that is invalid, but I'm cautious to say it's impossible to get one into a bad state. That's partly why I exposed the GpsTime::is_valid function, so users could double check that a time object is in a usable state.

I'd advise against converting the GpsTime into a seconds representation to perform the comparison since that could introduce unexpected behavior due to floating point rounding. This week's number of seconds since the start of GPS time is 2218 * 604800 = 1341446400, add on 3 more decimal places for milliseconds and we're at 13 decimal places compared to the 15-17 decimal place precision of a f64. That's a little close to the limit for me. We've seen some issues with the current C++ comparison which takes the difference between two times as a double in units of seconds. Maybe something like this would be preferable by keeping both weeks and time of week separated?

pub fn total_cmp(&self, other: & GpsTime) -> Ordering {
    if self.wn != other.wn {
        self.wn.cmp(&other.wn)
    } else {
        self.tow.total_cmp(&other.tow)
    }
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

The PartialOrd impl seems to always return Some, which makes me feel like we do in fact have a total ordering.

That's a good catch, we should probably re-work the PartialOrd implementation to handle NaNs and non-finite values better. Probably in a different PR

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd advise against converting the GpsTime into a seconds representation to perform the comparison since that could introduce unexpected behavior due to floating point rounding. This week's number of seconds since the start of GPS time is 2218 * 604800 = 1341446400, add on 3 more decimal places for milliseconds and we're at 13 decimal places compared to the 15-17 decimal place precision of a f64. That's a little close to the limit for me. We've seen some issues with the current C++ comparison which takes the difference between two times as a double in units of seconds. Maybe something like this would be preferable by keeping both weeks and time of week separated?

pub fn total_cmp(&self, other: & GpsTime) -> Ordering {

    if self.wn != other.wn {

        self.wn.cmp(&other.wn)

    } else {

        self.tow.total_cmp(&other.tow)

    }

}

Ahh right, that makes sense.

That's a good catch, we should probably re-work the PartialOrd implementation to handle NaNs and non-finite values better. Probably in a different PR

Sounds good. So I guess for now we can add a total_cmp ala f64 in the stdlib (with the separate wn/tow comparison like you said). And if we're ever able to "prove" that a GpsTime never contains a nan value we can add an Ord impl (not sure how practical that really is)

Copy link
Collaborator

Choose a reason for hiding this comment

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

That sounds good to me! Let's also make clear in the function comment that the time of week comparison mirrors the total_cmp in the standard library and has the same differences in behavior.

if self.wn() != other.wn() {
self.wn().cmp(&other.wn())
} else {
let mut left = self.tow().to_bits() as i64;
Copy link
Contributor

Choose a reason for hiding this comment

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

@adrian-kong could you stick a comment linking the original source, maybe a github permalink? I think technically we need to actually add a copy of their license into the repo too. And we should probably stick a TODO: replace with f64::total_cmp

@jbangelo I recommend we copy/paste the function because it was stabilized in 1.62, which apparently isn't available yet with the GH actions we are using. Plus it avoids having to update a bunch of other projects.

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 any specific convention for urls / license

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it be better to hide this behind a feature for now? Or is removing features later problematic as well?

@silverjam silverjam merged commit 1b3b54d into master Oct 22, 2022
@silverjam silverjam deleted the adrian/totalcmp branch October 22, 2022 00:47
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.

4 participants