Implement ops for primitives + Ratios; ops for BigRational + primitives #38

turboladen · 2018-08-21T06:01:20Z

Before this I was not able to use a primitive as the left-hand side of a core::ops::{Add, Sub, Mul, Div} method where the right-hand side was a Ratio of some type; this implements those. Also, I was not able to use a BigRational on the LHS and a primitive on the RHS of a core::ops::{Add, Sub, Mul, Div} method; this implements those as well (although I'm not sure I'm doing it in the most efficient code). I'm hoping these changes are welcome; I'm pretty new to the num crates and feel like I may have been missing something that should've been obvious.

Initially I'd manually implemented most of this stuff (not using generics), leading me to write a bunch of tests for all of the combinations of types, then refactored to use generics. Thus there's a bunch of tests that should cover all of the cases (hopefully not too many!). In doing so, it really started to feel like maybe some of the ops code could live in a separate file (mainly due to all of the tests), but since I'm not too familiar with this repo I decided to not rock the boat too much.

I've also done very little Rust work with others, so I may be missing some conventions--sorry about that! Please do let me know so I can tidy up accordingly.

These changes let you use a primitive on the LHS of an op statement where the RHS is a `Ratio`. This also lets you use primitives on the RHS of a `BigRational` equation.

turboladen · 2018-08-21T18:25:59Z

Ok, I've got all builds except for the 1.15.0 one passing; I'm not sure how to deal with those {i,u}128 failures though. Any tips would be quite helpful.

cuviper · 2018-08-21T18:42:55Z

When guarding code, please use #[cfg(has_i128)] instead of by feature, so it's still enabled by the build-script detection. For integer literals like 0u128, even guarded code becomes a problem since it still has to parse by the old compiler. You can let type inference handle it in some cases, like if calling fn foo(x: u128) then foo(0) is fine. In other cases you might have to assign a local first, let x: u128 = 0, which still parses on the old compiler even though it doesn't know that type.

turboladen · 2018-08-21T21:10:50Z

Ah, thank you @cuviper, that was super helpful (and makes total sense). Fixed all of those.

The other failures seem to have to do with isize::from() not being implemented for u8 and i16 until 1.26.0. I'm not really sure how to fix those tests (they seem relevant for current versions) so I've just commented them out (don't really like doing that) and left a TODO. I can remove these or whatever seems preferable, just let me know.

Since `BigRational`s/`BigInt`s don't implement `Copy`, seems like it'd be nice to be able to pass a reference on `100 / &some_big_rational`.

Similar to the previous commit, this extends behavior of letting Ratios operate on primitives (and BigInt). In the case of `BigRational`, I imagine references to them will be more prevalent than the other types, hence this commit.

…ational/&BigRational Also refactored related tests to use macros for defining inner test functions.

turboladen · 2018-09-01T00:33:40Z

Are there any problems with this PR? Just wondering as I have some other changes I'm curious about making, but they sort of build on the changes here and I don't want to get too far along there if there are problems here. TIA

cuviper · 2018-09-01T00:52:15Z

No problems AFAIK -- I was just on vacation and haven't come back to look at this again. I'll try to review it soon!

turboladen · 2018-09-01T00:54:58Z

@cuviper oh, nice! I mostly just wanted to give it a bump so it doesn't die. Thanks!

cuviper

First off, I'm sorry that I took so long to properly review this!

cuviper · 2018-10-02T18:08:49Z

src/lib.rs

+    (Ratio<T> for $primitive:ident) => {
+        impl<T> Add<Ratio<T>> for $primitive
+        where
+            T: Clone + Integer + ::core::convert::From<$primitive>,


I think adding a blanket impl<T> may be a breaking change, e.g. if someone already had their own impl Add<Ratio<Foo>> for i32. Honestly, I still get confused about exactly what the coherence rules would allow, but I think this would be legal for someone else to do, and your change would break it.

But regardless, I don't think we want implicit conversion between primitives. Note that the op implementations in the standard library don't do this conversion either. Idiomatic Rust is that primitive types should be explicitly converted as needed.

On the other hand, BigInt and BigUint do implement ops for the primitive types, and this is for performance reasons. They can entirely avoid allocating a new bigint buffer in most cases. However, here you defeat that idea if you use T::from here, because that forces a new allocation for that bigint T.

So I think the implementations we do want are:

impl Add<Ratio<$primitive>> for $primitive

impl Add<BigRational> for $primitive

impl Add<BigRational> for BigInt

could do Ratio<BigUint> too, but we're probably lacking in a lot of places for that...

& variations of all of these -- ref+value, value+ref, ref+ref

cuviper · 2018-10-02T18:14:00Z

src/lib.rs

+
+            #[inline]
+            fn add(self, rhs: Ratio<T>) -> Ratio<T> {
+                Ratio::new(rhs.numer + T::from(self), rhs.denom)


As noted above, you lose the bigint performance win if you force T::from here. But this implementation is also incorrect. Look at the arith_impl! macro -- when adding an integer to a ratio, you need to multiply it by the denominator! e.g. your code would compute 1/3 + 1 = 2/3, but it should be 4/3.

cuviper · 2018-10-02T18:14:48Z

src/lib.rs

+
+            #[inline]
+            fn sub(self, rhs: Ratio<T>) -> Ratio<T> {
+                Ratio::new(T::from(self) - rhs.numer, rhs.denom)


Similar to Add, we don't want T::from, and the math is incorrect.

cuviper · 2018-10-02T18:20:25Z

src/lib.rs

+impl_primitive_ops_ratio!(Ratio<T> for BigInt);
+
+#[cfg(feature = "bigint")]
+macro_rules! impl_ops_bigrational_for_float {


Hmm. I don't think it's a good idea to make float-conversions implicit, especially since you have to output an Option. That makes it pretty unusual from what most people would expect from ops. I think it's better to leave this one to the user to explicitly do their float->ratio conversion before attempting ops.

cuviper · 2018-10-02T18:21:06Z

src/lib.rs

+            }
+        }
+    };
+}


What about Rem?

cuviper · 2018-10-02T18:26:53Z

src/lib.rs

-    fn description(&self) -> &'static str {
-        match *self {
+    fn description(self) -> &'static str {
+        match self {


Why did you make this change? It's not really a problem either way, but it's totally unrelated to the rest of this PR.

I believe clippy caught this, and since it was minor I thought I'd go ahead and fix. I think there were a few more things that clippy suggested, but I held off on those since they were bigger changes. I'll keep these things out in the future.

I figured... clippy means well, but this code is on the error path, unlikely to ever make any difference to performance. No worries though, the change is harmless.

Ahh, yeah good point.

cuviper · 2018-10-02T18:28:37Z

src/lib.rs

-        use super::super::{Ratio, Rational};
+        #[cfg(feature = "bigint")]
+        use super::super::BigRational;
+        use super::super::{Ratio, Rational, Rational32, Rational64};


It's great that you're adding so many tests -- actually impressive that this makes up the bulk of your PR! However, src/lib.rs is getting unwieldy. Can we move your additions to a new submodule in a separate file?

Absolutely. I wanted to do that, but didn't wanna rock the boat too much with my first PR here. :)

cuviper · 2018-10-02T18:35:05Z

src/lib.rs

+            // }
+
+            test_isize(-2, _1, _NEG1);
+            test_i8(-2, _1, _NEG1);


Since you've only tested with integers here and elsewhere, the math problems I mentioned with Add and Sub didn't come into question. (The denominators here are just 1.)

turboladen · 2018-10-03T17:44:49Z

@cuviper thanks for the review! I hope to get to responding to all the things in the new few days. 🍻

turboladen · 2019-03-12T07:08:06Z

I just remembered this PR and wanted to pop in and say that I had to backlog the work I was doing that depended on these changes. That work is still in my (day job) backlog but I’m not sure when I’ll get time to pick it back up.

PTaylor-us · 2020-05-05T21:37:23Z

Just wanted to give this a bump as I noticed the lack of primitive support as well. I have been creating Ratios from the integers to to the the math, but I don't think it's a great solution. If it would help, I might be able to take a look at the requested changes for this PR and see what I can do.

turboladen added 4 commits August 20, 2018 22:49

Implement ops for primitives + Ratios

84ece06

These changes let you use a primitive on the LHS of an op statement where the RHS is a `Ratio`. This also lets you use primitives on the RHS of a `BigRational` equation.

Fix a clippy

b02bdd0

cargo fmt

99b4b4e

Fix travis failures for all but 1.15.0

6437862

Fix travis build failures for 1.15.0

e4a8627

turboladen added 5 commits August 21, 2018 15:43

impl {Add,Sub,Mul,Div}<Ratio<T>> for BigInt

7483ae8

impl {Add,Sub,Mul,Div}<&BigRational> for primitives

656fd4f

Since `BigRational`s/`BigInt`s don't implement `Copy`, seems like it'd be nice to be able to pass a reference on `100 / &some_big_rational`.

impl {Add,Sub,Mul,Div} for &BigRational

fe7d5c0

Similar to the previous commit, this extends behavior of letting Ratios operate on primitives (and BigInt). In the case of `BigRational`, I imagine references to them will be more prevalent than the other types, hence this commit.

impl {Add,Sub,Mul,Div}<BigRational/&BigRational/float> for float/BigR…

9eb9b74

…ational/&BigRational Also refactored related tests to use macros for defining inner test functions.

cargo fmt

d1c392e

turboladen force-pushed the feature/implement_ops_for_primitives branch from 44c61da to d1c392e Compare August 26, 2018 23:45

Add bigint feature flag for BigRational-related macro

7c80322

cuviper requested changes Oct 2, 2018

View reviewed changes

zbrachinara mentioned this pull request Mar 30, 2023

Symmetry for Ratio::new_raw #115

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement ops for primitives + Ratios; ops for BigRational + primitives #38

Implement ops for primitives + Ratios; ops for BigRational + primitives #38

turboladen commented Aug 21, 2018

turboladen commented Aug 21, 2018

cuviper commented Aug 21, 2018

turboladen commented Aug 21, 2018

turboladen commented Sep 1, 2018

cuviper commented Sep 1, 2018

turboladen commented Sep 1, 2018

cuviper left a comment

cuviper Oct 2, 2018

cuviper Oct 2, 2018

cuviper Oct 2, 2018

cuviper Oct 2, 2018

cuviper Oct 2, 2018

cuviper Oct 2, 2018

turboladen Oct 3, 2018

cuviper Oct 3, 2018

turboladen Oct 3, 2018

cuviper Oct 2, 2018

turboladen Oct 3, 2018

cuviper Oct 2, 2018

turboladen commented Oct 3, 2018

turboladen commented Mar 12, 2019

PTaylor-us commented May 5, 2020

Implement ops for primitives + Ratios; ops for BigRational + primitives #38

Are you sure you want to change the base?

Implement ops for primitives + Ratios; ops for BigRational + primitives #38

Conversation

turboladen commented Aug 21, 2018

turboladen commented Aug 21, 2018

cuviper commented Aug 21, 2018

turboladen commented Aug 21, 2018

turboladen commented Sep 1, 2018

cuviper commented Sep 1, 2018

turboladen commented Sep 1, 2018

cuviper 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turboladen commented Oct 3, 2018

turboladen commented Mar 12, 2019

PTaylor-us commented May 5, 2020