Skip to content
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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

YeungOnion
Copy link
Contributor

  • 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

Copy link

codecov bot commented Oct 24, 2024

Codecov Report

Attention: Patch coverage is 72.25434% with 192 lines in your changes missing coverage. Please review.

Project coverage is 92.57%. Comparing base (993a4b5) to head (3844e92).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
src/statistics/traits.rs 11.11% 32 Missing ⚠️
src/distribution/hypergeometric.rs 20.00% 20 Missing ⚠️
src/distribution/fisher_snedecor.rs 33.33% 18 Missing ⚠️
src/distribution/weibull.rs 63.15% 14 Missing ⚠️
src/distribution/bernoulli.rs 43.47% 13 Missing ⚠️
src/distribution/pareto.rs 47.36% 10 Missing ⚠️
src/distribution/students_t.rs 44.44% 10 Missing ⚠️
src/distribution/chi_squared.rs 55.55% 8 Missing ⚠️
src/distribution/erlang.rs 50.00% 8 Missing ⚠️
src/distribution/beta.rs 79.41% 7 Missing ⚠️
... and 18 more
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.
📢 Have feedback on the report? Share it here.

@YeungOnion YeungOnion marked this pull request as ready for review October 24, 2024 18:03
@YeungOnion
Copy link
Contributor Author

@FreezyLemon this removes the statistics::DiscreteDistribution and statistics::Distribution traits.
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.

Copy link
Contributor

@FreezyLemon FreezyLemon left a 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.

mod multivariate {
use nalgebra::{Cholesky, Dim, OMatrix, OVector, U1};

impl<D> super::Covariance<f64> for OVector<f64, D>
Copy link
Contributor

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

@YeungOnion
Copy link
Contributor Author

Would be nice to have the Never ! type, but that's not stable. Will do this for now. I'll write out the tests next.

- remove abandoned impls, add in the new ones where appropriate
- small doc updates, but not doctests
- updates tests to compile with new API
@YeungOnion
Copy link
Contributor Author

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 ast-grep and it was useful for the rewrite.

@YeungOnion
Copy link
Contributor Author

I also dropped the sparse trait method from the Covariance trait and renamed the trait to CovarianceMatrix perhaps there's another name more suitable, but the intent was to support various types as if they expressed covariance as an operator and how it acts on states. It felt a little ambiguous across types unlike the dense method.

@YeungOnion
Copy link
Contributor Author

Not sure what I'm misunderstanding about colorize/whiten and its testing regarding this.
Since square roots of matrices aren't unique, I'm relying on the quadratic forms like seen in distribution functions that have covariance matrices, like in the gaussian $-\frac{1}{2}x^T \Sigma^{-1}x$
This is also how scipy does their tests

@YeungOnion YeungOnion added this to the 0.18 milestone Nov 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants