-
Notifications
You must be signed in to change notification settings - Fork 85
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
replace [Discrete]Distribution trait for traits for moments and entropy separately #304
base: master
Are you sure you want to change the base?
Conversation
YeungOnion
commented
Sep 26, 2024
- feat: define new traits Covariance, Entropy, and CentralMoment
- feat!: replace Distribution with new traits for binomial
- feat!: replace Distribution with new traits for Gamma
- feat!: replace Distribution with new traits for uniform
- feat!: replace DiscreteDistribution for new traits for negative binomial
- feat!: replace Distribution for new traits with F-distribution
- feat!: replace Distribution for new traits for laplace
- feat!: replace Distribution for new traits for normal
- feat!: replace Distribution for new traits for multivariate vector distributions
e0dabb4
to
88368b8
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #304 +/- ##
==========================================
- Coverage 93.73% 92.57% -1.17%
==========================================
Files 53 53
Lines 12010 12174 +164
==========================================
+ Hits 11258 11270 +12
- Misses 752 904 +152 ☔ View full report in Codecov by Sentry. |
88368b8
to
c8ed456
Compare
@FreezyLemon this removes the |
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.
Looks really good, I like this API.
I'm relying a bit on unimplemented here - was wondering if I should have meaningful associated types for those trait methods as some are unit.
Hm.. good question. Setting the correct type feels right, but might be unexpected for a user because the unimplemented!() macro will only panic at runtime.
Probably better to set it to ()
for now, and maybe comment that it's unimplemented ("should be f32 but is unimplemented")? Something like that
Edit: The #[deprecated("...")]
attribute could also be (mis)used to denote a function as unimplemented. That would create a compile-time warning which should be fairly easy to spot.
src/statistics/traits.rs
Outdated
mod multivariate { | ||
use nalgebra::{Cholesky, Dim, OMatrix, OVector, U1}; | ||
|
||
impl<D> super::Covariance<f64> for OVector<f64, D> |
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.
Should probably have a few tests for this
c8ed456
to
f4d58a7
Compare
Would be nice to have the Never |
- remove abandoned impls, add in the new ones where appropriate - small doc updates, but not doctests - updates tests to compile with new API
f4d58a7
to
3844e92
Compare
Okay, I'll write the tests for Covariance now, but I realized that I didn't have to find a way to express in associated type if something is unimplemented, I could just separate the traits. Aside: I learned about |
internally relies on RealField
I also dropped the |
Not sure what I'm misunderstanding about colorize/whiten and its testing regarding this. |