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

Rename cov_diag to var? #124

Closed
devmotion opened this issue Mar 30, 2021 · 3 comments · Fixed by #125
Closed

Rename cov_diag to var? #124

devmotion opened this issue Mar 30, 2021 · 3 comments · Fixed by #125

Comments

@devmotion
Copy link
Member

Distributions.jl uses var for the diagonal of the covariance matrix and, e.g., defines a fallback for AbstractMvNormal: https://github.com/JuliaStats/Distributions.jl/blob/35125579a77b938f8d4a8bd2be23cbf1f2ddf225/src/multivariate/mvnormal.jl#L258

So to avoid that users end up with surprisingly inefficient code we could

  • define Statistics.var(gp::FiniteGP) = cov_diag(gp)
  • or rename cov_diag to var

What would you prefer?

@theogf
Copy link
Member

theogf commented Mar 30, 2021

Renaming cov_diag to Statistics.var sounds great to me!

@willtebbutt
Copy link
Member

willtebbutt commented Mar 30, 2021

If we were to do this, I'd want to rename all of the places where we refer to diagonals, such as mean_and_cov_diag -> mean_and_var, to ensure that we're consistent.

@devmotion
Copy link
Member Author

Yes, I agree, I thought about this as well and wanted to suggest it but somehow forgot to mention it above 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants