Skip to content

feat: accessor methods to various distribution#316

Merged
YeungOnion merged 4 commits intostatrs-dev:masterfrom
Qazalbash:issue-186
Apr 8, 2025
Merged

feat: accessor methods to various distribution#316
YeungOnion merged 4 commits intostatrs-dev:masterfrom
Qazalbash:issue-186

Conversation

@Qazalbash
Copy link
Contributor

@Qazalbash Qazalbash commented Jan 15, 2025

Hi, this PR fixes #186. I have implemented the accessor methods for the following distributions,

  • Dirac
  • Discrete Uniform
  • Log Normal
  • Triangular
  • Uniform
  • Multivariate Normal

@YeungOnion
Copy link
Contributor

YeungOnion commented Jan 16, 2025

Thanks for working on this. I think it all looks good, but I'm not sure if it's best to leak the implementation of multivariate normal with precision and cov_chol

If you can make these return cloned/owned values, it will be less binding in our API - there is no implication that the struct owns such data. While I have no expectations for this to change, but we're still fairly new.

As far as cost goes, the intended matrix sizes for nalgebra are small (dim<5) will be cheap for potential alloc and copies, so an accessor with cloning is cheaper than computing cholesky decomp or inversion.

A possible intermediate for those who need the precomputed values as is: a take method that returns a struct where all fields are pub containing the same fields as the "taken" MultivariateNormal but none of statistics impls.


EDIT

Quoting myself in #186

non-Copy fields should not clone for immutable accessor, clones should be opt-in by caller

And I just realized that I'm giving you mixed messages on copy being opt-in, aren't I?

🤔 Hmm. I think I need someone else's input here. In what cases might you want to view the precompute outside of the features we have already implemented?

@Qazalbash
Copy link
Contributor Author

Hi, I will keep an eye on this PR. Let me know the conclusion.

@YeungOnion
Copy link
Contributor

The precompute for the precision matrix is fairly common, so I'm no longer concerned that it's leaking implementation unnecessarily, so a borrow seems good there.

I don't know if it's common enough to store the cholesky $L$, it likely is common. However, the ambiguity I'm wanting to avoid is in whether we would have OMatrix for $L$, or if we would have the Cholesky type. I don't want to revert the change later, so can we hold off on an accessor for it until we get the specific need.

@YeungOnion
Copy link
Contributor

Hey, sorry just getting back to this.

Would you change the cholesky decomp "accessor" to be named, clone_cov_chol_decomp and make a clone instead?

This way it can be there if it's convenient but we aren't tied into having that precompute represented exactly by that type.

@Qazalbash
Copy link
Contributor Author

Hi, I have updated the name along with relevant refactoring.

@codecov
Copy link

codecov bot commented Apr 7, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.27%. Comparing base (a8fe65c) to head (f0502dd).
Report is 29 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #316      +/-   ##
==========================================
+ Coverage   93.81%   95.27%   +1.45%     
==========================================
  Files          53       60       +7     
  Lines       11996    14283    +2287     
==========================================
+ Hits        11254    13608    +2354     
+ Misses        742      675      -67     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@YeungOnion
Copy link
Contributor

Great! Thanks for this

@YeungOnion YeungOnion merged commit 4cc5200 into statrs-dev:master Apr 8, 2025
9 of 10 checks passed
@Qazalbash Qazalbash deleted the issue-186 branch April 8, 2025 19:34
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.

create accessors for all distributions

2 participants