Reduce allocations of cov(::MultivariateMixture)

[devmotion]

The PR reduces allocations of cov(::MultivatiateMixture) (addresses a TODO item).

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.27%. Comparing base (f33cc10) to head (a065a65).
Report is 2 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1967      +/-   ##
==========================================
+ Coverage   86.26%   86.27%   +0.01%     
==========================================
  Files         146      146              
  Lines        8763     8778      +15     
==========================================
+ Hits         7559     7573      +14     
- Misses       1204     1205       +1     

☔ 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.

[devmotion]

Would you be able to quickly review this small PR @jishnub?

[jishnub]

Looks fine to me. I suppose copytri! is internal to LinearAlgebra, but there isn't a public function to perform this operation.

[jishnub]

Should the pi be cast to the appropriate type here? I think BLAS expects floating-point numbers. But then, if tests pass, probably this is fine.

[devmotion]

In principle, I'd like to avoid BLAS calls even though md and V are (currently) Arrays. But it seems mul!(::Symmetric, ::Vector, ::Adjoint, ...) is not supported and according to a benchmark on my machine repeated generic mul! calls can be slower than syr! and a single copytri!... Everything apart from pi is hardcoded to Float64 currently though (something to be fixed as well), so

Suggested change

	BLAS.syr!('U', pi, md, V)
	BLAS.syr!('U', Float64(pi), md, V)

should make it safe.

[jishnub]

Yes, I agree that it is a bit unfortunate that there isn't a higher level API for either of these. This should be fine for now with the change above.

[jishnub]

Is there a test to cover this line? The fact that the tests passed without this change seems to suggest that this isn't covered.

[devmotion]

I'll check. I assume that - as in many tests - only distributions with Float64 parameters are tested. Here, eg, tests with a multivariate mixture with mixture probabilities of type Float64 would have passed even without the change.

[devmotion]

That was indeed the case, it was only tested with pi::Float64. I added a test with Float32 in a065a65 that would error without the Float64(pi) change.

[devmotion]

I suppose copytri! is internal to LinearAlgebra, but there isn't a public function to perform this operation.

Indeed, I used it only since Distributions already uses it in a few other places.