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
feat!: Update strum
and ordered-float
dependencies and change From<LogProb> into TryFrom<LogProb> for NotNan<f64>.
#491
Changes from 9 commits
2f1b29e
dd2db90
a42e59d
c30ce2f
080b9f7
096e156
aaffd64
0ad0658
c52604d
aa90c4e
99f2ba8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,7 @@ | |
|
||
use std::cmp; | ||
use std::collections::HashMap; | ||
use std::convert::Into; | ||
use std::convert::{Into, TryFrom}; | ||
use std::hash::Hash; | ||
use std::{ | ||
fmt::Debug, | ||
|
@@ -43,7 +43,7 @@ use ordered_float::NotNan; | |
/// ); | ||
/// abs_diff_eq!(integral.exp(), 0.682, epsilon=0.01); | ||
/// ``` | ||
pub fn ln_integrate_exp<T, F>( | ||
pub fn ln_integrate_exp<T, F, E>( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think a PR changing the signature of a public method like this (which, granted, in most cases won't actually affect most users because the type will just be inferred, but will affect anyone who needs to specify any of the generic parameters) should probably not be labeled "chore", since "chore" PRs don't show up in the changelog. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would you classify it at a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it should be feat: actually, since the API is extended with a new feature here (allowing to have a fallible conversion from f64) |
||
mut density: F, | ||
min_point: T, | ||
max_point: T, | ||
|
@@ -57,10 +57,11 @@ where | |
+ Div<NotNan<f64>, Output = T> | ||
+ Mul<Output = T> | ||
+ Into<f64> | ||
+ From<f64> | ||
+ TryFrom<f64, Error = E> | ||
+ Ord | ||
+ Debug | ||
+ Hash, | ||
E: Debug, | ||
F: FnMut(T) -> LogProb, | ||
f64: From<T>, | ||
{ | ||
|
@@ -112,7 +113,7 @@ where | |
// METHOD additionally investigate small interval around the optimum | ||
for point in linspace( | ||
cmp::max( | ||
middle.unwrap() - (max_resolution.into() * 3.0).into(), | ||
T::try_from(middle.unwrap() - max_resolution.into() * 3.0).unwrap(), | ||
min_point, | ||
) | ||
.into(), | ||
|
@@ -124,23 +125,23 @@ where | |
linspace( | ||
middle.unwrap().into(), | ||
cmp::min( | ||
middle.unwrap() + (max_resolution.into() * 3.0).into(), | ||
middle.unwrap() + T::try_from(max_resolution.into() * 3.0).unwrap(), | ||
max_point, | ||
) | ||
.into(), | ||
4, | ||
) | ||
.skip(1), | ||
) { | ||
grid_point(point.into(), &mut probs); | ||
grid_point(T::try_from(point).unwrap(), &mut probs); | ||
} | ||
|
||
let sorted_grid_points: Vec<f64> = probs.keys().sorted().map(|point| (*point).into()).collect(); | ||
|
||
// METHOD: | ||
// Step 2: integrate over grid points visited during the binary search. | ||
LogProb::ln_trapezoidal_integrate_grid_exp::<f64, _>( | ||
|_, g| *probs.get(&T::from(g)).unwrap(), | ||
|_, g| *probs.get(&T::try_from(g).unwrap()).unwrap(), | ||
&sorted_grid_points, | ||
) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,7 @@ pub mod adaptive_integration; | |
pub mod cdf; | ||
pub mod errors; | ||
|
||
use std::convert::TryFrom; | ||
use std::f64; | ||
use std::iter; | ||
use std::mem; | ||
|
@@ -18,7 +19,7 @@ use std::ops::{Add, AddAssign, Div, Mul, Sub, SubAssign}; | |
use itertools::Itertools; | ||
use itertools_num::linspace; | ||
use num_traits::{Float, Zero}; | ||
use ordered_float::NotNan; | ||
use ordered_float::{FloatIsNan, NotNan}; | ||
|
||
use crate::utils::FastExp; | ||
|
||
|
@@ -401,9 +402,10 @@ impl From<NotNan<f64>> for LogProb { | |
} | ||
} | ||
|
||
impl From<LogProb> for NotNan<f64> { | ||
fn from(p: LogProb) -> NotNan<f64> { | ||
NotNan::from(*p) | ||
impl TryFrom<LogProb> for NotNan<f64> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry I was unclear but I would actually probably recommend leaving the (potentially-panicing) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. unfortunately you don't seem to be able to implement a deprecated There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I prefer the tryfrom here, let's make this a breaking change (feat!:), then. |
||
type Error = FloatIsNan; | ||
fn try_from(p: LogProb) -> Result<Self, Self::Error> { | ||
NotNan::new(*p) | ||
} | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once we're updating this... would it be possible to make this
There were breaking changes in 3.0.0, but if they don't break this crate then it would be preferable to allow users to decide which version they want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds reasonable to me. @adam-azarchs are you sure that the breaking changes don't affect us here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not 100%. They don't look like behavior changes, just some dropped implementations, so it should be easy enough to just try it and see if it compiles. I know for our internal purposes we're not going to be updating to 3.0 particularly soon but future proofing is good.