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

Timestamp comparison is wrong #1037

Closed
magmax opened this issue May 27, 2024 · 0 comments · Fixed by #1038
Closed

Timestamp comparison is wrong #1037

magmax opened this issue May 27, 2024 · 0 comments · Fixed by #1038

Comments

@magmax
Copy link
Contributor

magmax commented May 27, 2024

Currently, samples.Timestamp implementation for comparisons is:

    def __gt__(self, other: "Timestamp") -> bool:
        return self.sec > other.sec or self.nsec > other.nsec

    def __lt__(self, other: "Timestamp") -> bool:
        return self.sec < other.sec or self.nsec < other.nsec

what might not compare correctly. Example:

>>> a = Timestamp(2, 5)
>>> b = Timestamp(3, 4)
>>> a > b
True

In the example, the first part is 2<3, what is false, forcing to perform the next part, what is 5>4, returning True.

A good implementation could be:

    def __gt__(self, other: "Timestamp") -> bool:
        if self.sec == other.sec:
            return self.nsec > other.nsec
        return self.sec > other.sec

    def __lt__(self, other: "Timestamp") -> bool:
        if self.sec == other.sec:
            return self.nsec < other.nsec
        return self.sec < other.sec

Another option, perhaps even better, is to rely on python's tuples implementation:

    def __gt__(self, other: "Timestamp") -> bool:
        return (self.sec, self.nsec) > (other.sec, other.nsec)

    def __lt__(self, other: "Timestamp") -> bool:
        return (self.sec, self.nsec) < (other.sec, other.nsec)
@magmax magmax changed the title Timestamp comparision is wrong Timestamp comparison is wrong May 27, 2024
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 a pull request may close this issue.

1 participant