-
Notifications
You must be signed in to change notification settings - Fork 428
Implement std(::MultivariateDistribution) #1352
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1352 +/- ##
==========================================
+ Coverage 86.25% 86.27% +0.01%
==========================================
Files 146 146
Lines 8766 8777 +11
==========================================
+ Hits 7561 7572 +11
Misses 1205 1205 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Seems reasonable! Can you also add std
to the documentation of multivariate distributions?
@bicycle1885 are you still interested in finishing this PR? @devmotion made a reasonable code suggestion that you would just need to accept, the other thing that seems missing is "add std to the documentation of multivariate distributions" - @devmotion could you clarify what you mean by that ? |
src/multivariates.jl
Outdated
function std(d::MultivariateDistribution) | ||
v = var(d) | ||
map!(sqrt, v, v) | ||
return v | ||
end |
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.
Unfortunately, it seems in general var(d)
might not be mutable - eg it can be a Fill
array (eg for Dirichlet
) or it could be a static array. So either
- we live with these sometimes unnecessary allocations in the general fallback until Julia provides a native way to distinguish mutable and immutable arrays (similar to ArrayInterface) and demand that one has to implement a more efficient alternative for specific distributions if possible, or
- we demand that
var
returns a mutable array in general and one has to implementstd
for distributions for whichvar
is immutable.
Personally, I think it might be better to have a more efficient default implementation and eg. add a fix for Dirichlet
.
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.
@mschauer or @andreasnoack do you think it's fine to add a special case for Dirichlet
or should it assume immutability by default? I suggested map!
to reduce allocations (#1352 (comment)) but this is problematic for Dirichlet
due since Fill
arrays are not mutable.
I addressed both. I also added a fix for |
@devmotion tests are passing, should it get merged now ? |
I don't want to approve and merge my own changes. This does not feel right. |
@devmotion @st-- in open source ppl write PRs then forget about them, moving on to other things ALL THE TIME! |
This implements
std
for multivariate distributions as an element-wise square root of variance:std(d) = sqrt.(var(d))
.