Implement Multivariate Normal Distribution with nalgebra #100
Conversation
…ate other dependencies
There was a problem hiding this comment.
Thanks for submitting a PR! I think this is good work and it looks like the nalgebra crate is in a state where we can safely depend on it for linear algebra/matrix operations.
That being said, my preference would be to provide an implementation of the multivariate normal distribution from within statrs itself rather than depending on nalgebra-mvn. This will allow us to provide an API that's more consistent with the rest of the distributions in the crate and also enable us to evolve the API in a more consistent manner since we won't have to depend on how the distribution is implemented in nalgebra-mvn.
I'm open to counter-arguments and questions though if you have any thoughts!
|
Ok, if you prefer that I'll work on implementing the rest of the functionality within statrs and remove nalgebra-mvn as a dependency. |
There was a problem hiding this comment.
This is excellent work! Only a couple things:
-
Could we add some tests? You can probably draw from Math.Net Numerics as an example, most of the other distribution tests draw from Math.NET for all if not the majority of their tests
-
I think from an API perspective it makes more sense to enforce
f64as the numerical type instead ofRealsince the rest of the distributions currently only supportf64. I think it makes more sense to introduce support for generic numerical types across the board in a separate effort rather than have the API diverge in this PR. I'm definitely open to counter-arguments/discussion though.
Overall super appreciative of the contribution!
| /// | ||
| /// where `μ` is the mean, `inv(Σ)` is the precision matrix, `det(Σ)` is the determinant | ||
| /// of the covariance matrix, and `k` is the dimension of the distribution | ||
| fn ln_pdf(&self, x: VectorN<Real, N>) -> Real { |
There was a problem hiding this comment.
Can this be expressed in terms of pdf?
|
|
I'm just about done with this, let me know if you think I should add more test cases. |
Cargo.toml
Outdated
| @@ -19,3 +19,5 @@ path = "src/lib.rs" | |||
|
|
|||
| [dependencies] | |||
| rand = "0.7" | |||
| nalgebra = "0.19" | |||
| num-traits = "0.2.10" | |||
There was a problem hiding this comment.
do we still need the explicit dependency on num-traits?
|
Should be all done but I don't think I have permission to merge? Not quite sure how this works... XD |
This uses another crate "nalgebra-mvn" as a dependency, which is on an older version of nalgebra that depends on an older version of rand. I've opened a PR to update it to nalgebra v0.19, but for now Cargo.toml reflects that I'm compiling with a local version of nalgebra-mvn.
My first PR, please comment! :D