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

Smarter comparisons #49

Closed
maxbla opened this issue Jun 14, 2019 · 2 comments
Closed

Smarter comparisons #49

maxbla opened this issue Jun 14, 2019 · 2 comments

Comments

@maxbla
Copy link
Contributor

maxbla commented Jun 14, 2019

Ratio{numer:0, denom:1} is not equal to Ratio{numer:0, denom:2}

The code that appears to deal with this is

// With equal numerators, the denominators can be inversely compared
if self.numer == other.numer {
    let ord = self.denom.cmp(&other.denom);
    return if self.numer < T::zero() {
        ord
    } else {
        ord.reverse()
    };
}

It seems like we could easily add a case for self.numer == T::zero(), but this ties in with #8 and I don't see a need to rush a fix.

@maxbla maxbla changed the title Smarter equals methods Smarter comparisons Jun 14, 2019
@cuviper
Copy link
Member

cuviper commented Jun 14, 2019

Is there a way that you created 0/2 besides new_raw? I think all other paths should reduce to 0/1 already.

Regardless, we should fix this, just as the general comparison doesn't assume they are reduced. Would you like to send a PR? Note that using self.numer.is_zero() is slightly better to avoid construction.

@maxbla
Copy link
Contributor Author

maxbla commented Jun 14, 2019

I encountered it in testing after removing a call to reduce() in AddAssign. I decided to not mess with reduce for now.

I would be happy to submit a PR for this, but I'd like to mostly finish #42 first.

@maxbla maxbla mentioned this issue Jun 17, 2019
bors bot added a commit that referenced this issue Jul 18, 2019
51: Fix #49 r=cuviper a=maxbla

fixed issue #49 
added a test case for it

Side note:
`cmp` is a bit scary. Do we know what it's amortized runtime is? It seems to me that the worst case (having to compare the reciprocals many times) could  be pretty bad. Is there any way to have a separate `impl` for `T: Clone + Integer + CheckedMul`? As the comments note, CheckedMul would make the implementation faster. I messed around, but I can't find a way to have two implementations -- one for checked and one for no checked. I found an [this](rust-lang/rfcs#586) RFC for negative trait bounds though (spoiler: negative trait bounds are not happening soon).

Co-authored-by: Max Blachman <blachmanmax@gmail.com>
@bors bors bot closed this as completed in e10ca81 Jul 18, 2019
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

No branches or pull requests

2 participants