Generalize stats from f64 to the Float trait #12606
Conversation
| fn percentile_of_sorted(sorted_samples: &[f64], | ||
| pct: f64) -> f64 { | ||
| fn percentile_of_sorted<T:Float + FromPrimitive>(sorted_samples: &[T], | ||
| pct: T) -> T { |
There was a problem hiding this comment.
This should align with the start of sorted_samples
|
I'd prefer this to wait until #12355 lands (which should be soon anyway). However, those precision problems are a little concerning, if the those tests are still working with (I haven't given a full review yet, but it seems a lot of the function arguments need re-indenting, and also, two style points that appear to come up a few times: <T: Float + FromPrimitive>
let null: T = ...;i.e. no space before |
| let med = self.median(); | ||
| let abs_devs = self.map(|&v| num::abs(med - v)); | ||
| // This constant is derived by smarter statistics brains than me, but it is | ||
| // consistent with how R and other packages treat the MAD. | ||
| abs_devs.median() * 1.4826 | ||
| let number = FromPrimitive::from_f32(1.4826).unwrap(); |
There was a problem hiding this comment.
I suspect this is the reason that the assertions were failing on median_abs_dev_pct. 1.4826 was previously f64, now it is f32 cast to T. Ideally, it should just be T. Does type inference for floating point literals not work as expected in generic functions (such that the original line could have been used as is)?
There was a problem hiding this comment.
Yes, it doesn't work in generic functions (as far as the compiler knows, something other than f32 and f64 could implement the Float trait, and the generic literals only work for those two types). I think making this just from_f64 would be fine. Or, even from_i32(14826) / from_i32(10000) for exactness.
There was a problem hiding this comment.
That is understandable, if somewhat unfortunate. The closest f32 to the desired decimal literal may not be the same as the result of converting from the nearest f64. Although, with the current str <--> floating point implementation, that is unlikely to be the biggest problem.
There was a problem hiding this comment.
Thanks for the reviews!
I haven't updated my pull request because I have some problems with failing tests.
|
Closing due to inactivity, but feel free to reopen with a rebase! |
|
I made a new pull request at #13822. Thanks for the reviews. |
I would like a review of my changes.
This is not ready for merging. Some tests fail because of some float rounding issues.
But I cannot find the reasons for the failures, I see only changes related to the generalizing.
The output is the following:
---- stats::tests::test_exp10a stdout ----
task 'stats::tests::test_exp10a' failed at '116.029565 is not approximately equal to 116.029563', /home/leifeld/workspace/src/rust/src/libextra/stats.rs:493
---- stats::tests::test_exp10b stdout ----
task 'stats::tests::test_exp10b' failed at '93.797143 is not approximately equal to 93.797141', /home/leifeld/workspace/src/rust/src/libextra/stats.rs:493
---- stats::tests::test_exp10c stdout ----
task 'stats::tests::test_exp10c' failed at '97.716818 is not approximately equal to 97.716816', /home/leifeld/workspace/src/rust/src/libextra/stats.rs:493
---- stats::tests::test_exp25 stdout ----
task 'stats::tests::test_exp25' failed at '101.441053 is not approximately equal to 101.441051', /home/leifeld/workspace/src/rust/src/libextra/stats.rs:493
---- stats::tests::test_norm10medium stdout ----
task 'stats::tests::test_norm10medium' failed at '195.7032 is not approximately equal to 195.703197', /home/leifeld/workspace/src/rust/src/libextra/stats.rs:492
---- stats::tests::test_norm10narrow stdout ----
task 'stats::tests::test_norm10narrow' failed at '102.2994 is not approximately equal to 102.299398', /home/leifeld/workspace/src/rust/src/libextra/stats.rs:492
---- stats::tests::test_norm10wide stdout ----
task 'stats::tests::test_norm10wide' failed at '611.5725 is not approximately equal to 611.572489', /home/leifeld/workspace/src/rust/src/libextra/stats.rs:492
---- stats::tests::test_unif25 stdout ----
task 'stats::tests::test_unif25' failed at '102.134667 is not approximately equal to 102.134665', /home/leifeld/workspace/src/rust/src/libextra/stats.rs:493