Add Inv and Pow traits. #37

clarfonthey · 2018-02-18T19:31:05Z

This is not a breaking change, and closes #34 and #38.

This doesn't add any impls for the other num crates, just floats with std enabled. The trait has to be added before those other crates can be updated.

cuviper

Thanks for the PR - it's a good start. Besides the comments below, can you also add some testing? A simple example in the documentation should suffice. Maybe also note that some types may panic on "invalid" input, like a divide-by-zero, non-square matrix, etc.

cuviper · 2018-02-18T23:17:52Z

src/ops/mod.rs

@@ -1,3 +1,4 @@
 pub mod saturating;
 pub mod checked;
 pub mod wrapping;
+pub mod new;


Should be pub mod inv;?

cuviper · 2018-02-18T23:28:34Z

src/ops/inv.rs

+
+            #[inline]
+            fn inv(self) -> $out {
+                ($fn)(self)


I don't this function call will actually work for the reference versions, and it's weird having a lifetime parameter on the value versions. There are only 4 implementations here, so maybe it's not worth trying to make a unified macro.

To avoid cfg(feature = "std"), we could just implement these as 1.0 / self. That's all the standard library is doing anyway.

I'll do that later today

clarfonthey · 2018-02-19T20:01:30Z

I figured I'd implement #38 in this as well considering how I was going to offer both, and I don't want to deal with merge conflicts. Let me know if everything looks alright!

cuviper · 2018-02-19T20:08:35Z

src/ops/pow.rs

+pow_impl!(isize, u32, pow);
+
+#[cfg(feature = "std")]
+mod float_impls {


You'll have to use super::Pow; to get it into this module.

…that is correct. I'll just factor out the cfg.

cuviper · 2018-02-19T20:10:16Z

Let's put Pow into the existing pow module, instead of under ops.

cuviper · 2018-02-22T21:48:01Z

src/ops/inv.rs

+    /// let x = 7.0;
+    /// let y = -0.0;
+    /// assert_eq!(x.inv() * x, One::one());
+    /// assert_eq!(y.inv() * y, One::one());


This is -inf * -0.0, which is actually NAN. Maybe just assert the -inf instead?

cuviper · 2018-02-22T21:50:20Z

src/pow.rs

+    ///
+    /// ```
+    /// use num_traits::Pow;
+    /// assert_eq!(10.pow(2), 100);


CI failed here because it inferred this like 10i32.pow(2i32), which is not implemented.

clarfonthey · 2018-02-23T22:45:59Z

Should be ready to merge now.

cuviper · 2018-02-23T23:02:56Z

Sorry if I misspoke, but I did not mean that we should have so many variations of Pow implementations. Do you have your own reasons for all them? I'm especially worried about the ones with lossy as casts. Just the basic natively-supported pow implementations looked good to me.

clarfonthey · 2018-02-23T23:05:27Z

@cuviper I accidentally added the u64 casts in error, but the rest are lossless casts (such as u8 as u32).

My main reasoning was to do something similar to how Shl is done in the standard library: https://doc.rust-lang.org/std/ops/trait.Shl.html

cuviper · 2018-02-23T23:47:40Z

Ah, I got confused by the macros, and thought you had some signed->unsigned casts in there too.

Can we do it without the closures though? I think if you leverage From/Into, then the final implementation can be just $method(self, rhs.into()), where $method is either the type's inherent pow or num's pow for usize. No as casting at all.

Should floats do u32 and usize with num's pow too? And perhaps also u8 and u16 this way under no_std (without powi)?

I count 372 Pow implementations so far in the generated docs! The downside is that all this does significantly increase the compilation time, compared to the current master branch.

clarfonthey · 2018-02-24T00:04:57Z

I think that doing without the closures is very fair and I'll refactor this in a bit to achieve that. Also I just noticed rust-lang/rust#48321 and I think that simply deferring to wrapping_pow once it's stable would be better for Wrapping, and that those impls can just be removed for now to avoid compile time hikes.

I'll see what I can do about benchmarking compile times and how that can be improved.

cuviper · 2018-02-24T00:26:24Z

To put real numbers on it, cargo build --release goes from 0.72 seconds to 1.3 seconds on my i7-7700k. That's still bearable, but quite a significant increase for just one feature.

clarfonthey · 2018-02-24T00:35:40Z

I don't think a double in compile time is acceptable; I'll see what I can do.

clarfonthey · 2018-02-27T19:10:42Z

@cuviper with the latest change, my compile time went from 2.29s to 1.84s, which is only a 20% increase. Could you provide benchmarks on your end?

Longer-term it may be best to make the standalone pow function private and change the usize exponent to a u64 for compatibility. This is breaking, obviously, and I didn't do that.

cuviper · 2018-02-27T19:56:16Z

For me it's now 0.86s vs master's 0.72s -- that seems fine. There are still a lot of implementations -- 292 -- so I'd guess the closures were the main culprit for the slowdown.

Let's merge!

bors r+

37: Add Inv and Pow traits. r=cuviper a=clarcharr This is not a breaking change, and closes #34 and #38. This doesn't add any impls for the other `num` crates, just floats with `std` enabled. The trait has to be added before those other crates can be updated.

bors · 2018-02-27T19:58:57Z

Build succeeded

continuous-integration/travis-ci/push

clarfonthey · 2018-02-27T19:59:33Z

Awesome! Could you push a new version of num-traits with these changes added?

cuviper · 2018-02-27T20:06:33Z

Yeah, but I still want to revisit FloatCore and perhaps add some more to it before it's published. I'll try to do this today.

clarfonthey · 2018-02-27T20:43:31Z

That's fair; no rush. :)

cuviper requested changes Feb 18, 2018

View reviewed changes

cuviper reviewed Feb 19, 2018

View reviewed changes

cuviper reviewed Feb 22, 2018

View reviewed changes

clarfonthey changed the title ~~Add Inv trait.~~ Add Inv and Pow traits. Feb 23, 2018

clarfonthey closed this Feb 24, 2018

clarfonthey reopened this Feb 24, 2018

clarfonthey added 7 commits February 27, 2018 13:25

Add Inv trait.

5bdff3f

Fix Inv trait, add Pow trait.

c1f4118

Move Pow to pow module.

ce3badc

Fix doc tests.

5d6933f

Add more Pow implementations.

61a6acc

Remove Pow<u64> (accidentally added).

aca8dc8

Ensure infalliability of conversions, avoid closures.

79b557f

cuviper approved these changes Feb 27, 2018

View reviewed changes

bors bot merged commit 79b557f into rust-num:master Feb 27, 2018

clarfonthey deleted the inv branch March 5, 2018 21:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Inv and Pow traits. #37

Add Inv and Pow traits. #37

clarfonthey commented Feb 18, 2018 •

edited

Loading

cuviper left a comment

cuviper Feb 18, 2018

clarfonthey Feb 19, 2018

cuviper Feb 18, 2018

clarfonthey Feb 19, 2018

clarfonthey commented Feb 19, 2018

cuviper Feb 19, 2018

clarfonthey Feb 19, 2018

cuviper commented Feb 19, 2018

cuviper Feb 22, 2018

cuviper Feb 22, 2018

clarfonthey commented Feb 23, 2018

cuviper commented Feb 23, 2018

clarfonthey commented Feb 23, 2018

cuviper commented Feb 23, 2018

clarfonthey commented Feb 24, 2018

cuviper commented Feb 24, 2018

clarfonthey commented Feb 24, 2018

clarfonthey commented Feb 27, 2018

cuviper commented Feb 27, 2018

bors bot commented Feb 27, 2018

clarfonthey commented Feb 27, 2018

cuviper commented Feb 27, 2018

clarfonthey commented Feb 27, 2018

Add Inv and Pow traits. #37

Add Inv and Pow traits. #37

Conversation

clarfonthey commented Feb 18, 2018 • edited Loading

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

clarfonthey commented Feb 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cuviper commented Feb 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clarfonthey commented Feb 23, 2018

cuviper commented Feb 23, 2018

clarfonthey commented Feb 23, 2018

cuviper commented Feb 23, 2018

clarfonthey commented Feb 24, 2018

cuviper commented Feb 24, 2018

clarfonthey commented Feb 24, 2018

clarfonthey commented Feb 27, 2018

cuviper commented Feb 27, 2018

bors bot commented Feb 27, 2018

Build succeeded

clarfonthey commented Feb 27, 2018

cuviper commented Feb 27, 2018

clarfonthey commented Feb 27, 2018

clarfonthey commented Feb 18, 2018 •

edited

Loading