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

feature: Implement Continuous<Vec<f64>> for MultivariateNormal #199

Merged

Conversation

saona-raimundo
Copy link
Contributor

Solves #194

The problem was the public dependency on nalgebra::Dvector:
Although statrs does not update its dependencies, users should easily sample from MultivariateNormal.

The users were using an updated version of nalgebra and the types had clashes.
Therefore, this implements the same trait for a type with guaranteed backward compatibility: Vec.

Maintaining or not the nalgebra::DVector in the public API is an open question that we did not touch.

@YeungOnion
Copy link
Contributor

Thanks for identifying a good solution here. I'm good to merge this, @henryjac thoughts?

@YeungOnion YeungOnion merged commit f5e238f into statrs-dev:master Apr 17, 2024
@YeungOnion
Copy link
Contributor

Hmm, I believe I merged this in a rush. I'd yet to get input here on #209 which reintroduces the coupling to internal and at API usage of nalgebra and I was considering the option for an multidimensional_nalgebra feature to allow using this without using an internal nalgebra which would be a taller order.

@saona-raimundo
Copy link
Contributor Author

As far as I could see, the problem of #194 (requiring the user to use possibly more than one version of nalgebra) is also present on #209
If this is the case, then I see no problem in accepting both interfaces: Vec can always be used, while nalgebra can be used easily if there is no dependency clashes.
Before we make it efficient or convenient, it should "just work", and I think Vec is needed in this case.

@YeungOnion
Copy link
Contributor

I think you're right that both will work for now as it (and similar for later ContinuousCDF) solves the usage of trait Continuous.

However, I think the traits that return OVector (e.g. MeanN, VarianceN, rand::distributions::Distribution) would return library constrained version of OVector. That sound right? Maybe that'd be another issue?

@saona-raimundo
Copy link
Contributor Author

Ah! You are right.
I would say they are different kind of problems.
One thing is the user needing to input an nalgebra struct of a specific version, while another is statrs returning an nalgebra struct of a specific version.
They both introduce a possible dependency problem, but they have different solutions.

I have not tested it but the second might not be much of a problem.
If the struct that statrs returns has a method to construct a Vec with the underlying data, and the user does not need to add a dependency for that, then this is not much of a problem.
We can mention it in the documentation.

@YeungOnion
Copy link
Contributor

Ah yes, it's on the other side. I'll write it as a separate issue, thanks for your input!

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