Remove pre-allocation of outputs for logpdf and pdf + deprecate implementations for multiple samples
#1257
Conversation
src/multivariates.jl
Outdated
| @@ -230,15 +230,13 @@ end | |||
| function logpdf(d::MultivariateDistribution, X::AbstractMatrix) | |||
| size(X, 1) == length(d) || | |||
| throw(DimensionMismatch("Inconsistent array dimensions.")) | |||
| T = promote_type(partype(d), eltype(X)) | |||
| _logpdf!(Vector{T}(undef, size(X,2)), d, X) | |||
| map(i -> _logpdf(d, view(X, :, i)), axes(X, 2)) | |||
There was a problem hiding this comment.
It would be nice to just use
| map(i -> _logpdf(d, view(X, :, i)), axes(X, 2)) | |
| map(Base.Fix1(_logpdf, d), eachcol(X)) |
here but eachcol requires Julia >= 1.1 or Compat.
There was a problem hiding this comment.
Indeed. I had forgotten that eachcol/row weren't available in 1.0.
src/multivariates.jl
Outdated
| end | ||
|
|
||
| function pdf(d::MultivariateDistribution, X::AbstractMatrix) | ||
| size(X, 1) == length(d) || | ||
| throw(DimensionMismatch("Inconsistent array dimensions.")) | ||
| T = promote_type(partype(d), eltype(X)) | ||
| _pdf!(Vector{T}(undef, size(X,2)), d, X) | ||
| map(i -> _pdf(d, view(X, :, i)), axes(X, 2)) |
There was a problem hiding this comment.
Same here, with eachcol one could write
| map(i -> _pdf(d, view(X, :, i)), axes(X, 2)) | |
| map(Base.Fix1(_pdf, d), eachcol(X)) |
|
Similar changes could (should?) be applied to |
Codecov Report
@@ Coverage Diff @@
## master #1257 +/- ##
==========================================
- Coverage 81.81% 81.61% -0.21%
==========================================
Files 117 117
Lines 6589 6505 -84
==========================================
- Hits 5391 5309 -82
+ Misses 1198 1196 -2
Continue to review full report at Codecov.
|
|
I deprecated all |
logpdf and pdflogpdf and pdf + deprecate implementations for multiple samples
|
I think this requires some additional discussion - maybe it would be better to revert the deprecations that were suggested by @andreasnoack and just fix the allocating calls for now? What's the general opinion? Ping @matbesancon @mschauer |
|
I'd be fine with splitting this into two PRs: one similar to your original version which could then be a bugfix release and then a PR similar to the current version that would require a breaking release but I still think it would be good to deprecate these methods. |
|
@devmotion should this be closed now? |
|
Seems it was addressed in the linked PRs 👍 |
This PR removes the pre-allocation of output arrays for
logpdfandpdf.There seems little reason to fall back to the in-place methods since it does neither reduce allocations nor lines of code. Instead, I'd argue that the pre-allocation causes problems since the computation of the output type is only a heuristic and it seems easy to come up with examples where it fails since typically
logpdfandpdfdo not ensure that their output is of this specific type. Additionally, the pre-allocations lead to calls of mutating functions which break AD with e.g. Zygote. Moreover, it is consistent withlogpdfandpdfimplementations for univariate distributions that do not fall back to the mutating functions but use broadcasting (one could use the same implementation withmapandBase.Fix1there as well) - however, it seems a bit strange that the use of(log)pdfwith arrays is deprecated for univariate distributions but not for multi- and matrixvariate distributions.