Conversation
|
These functions are usually useful for me, thought it'd be nice to have them here. Not sure why CI is failing on Julia 1.0 ? |
| function logmeanexp(A::AbstractArray; dims=:) | ||
| R = logsumexp(A; dims) | ||
| N = length(A) ÷ length(R) | ||
| return R .- log(N) |
There was a problem hiding this comment.
This will cause undesired promotions and allocations.
There was a problem hiding this comment.
I think it's better now.
There was a problem hiding this comment.
It seems you adressed my comment about promotions but not the allocations.
There was a problem hiding this comment.
@devmotion you might argue that unless there's a way to modify logsumexp to directly take log(N) into account, an in-place operation might create issues with AD systems which do not support it?
| R = logsumexp(2logsubexp.(A, logmean); dims) | ||
| N = length(A) ÷ length(R) | ||
| if corrected | ||
| return R .- log(N - 1) |
There was a problem hiding this comment.
Again this causes undesired promotions and allocations.
There was a problem hiding this comment.
Thanks, I think it's better now.
There was a problem hiding this comment.
The unnecessary allocations are still present.
There was a problem hiding this comment.
Then I'm not sure what you are referring to?
There was a problem hiding this comment.
R .- log(N - 1)creates a new array. But if R is e.g. of type Array this causes unnecessary allocations.
|
I don't have an strong opinion on this, but Is it possible to implement this as an extension? This package is a really low level one, and adding a new dependency could have effects on downstream packages? |
|
@longemen3000 The PR is not introducing any dependencies. |
|
|
|
Ohhh, nvm, my error, in that case, no comment at all |
|
@longemen3000 Yes but |
|
CI passing now. @devmotion I agree the implementation is not optimal but I don't have time to tune it now. Could we merge as it is now? |
|
I think we can merge now and work on optimization later. Honestly, it's just a huge pain trying to optimize for allocations without having Transducers.jl, laziness, or good functional primitives in Julia. |
|
@ParadaCarleton as far as I know you've never contributed to this package, so I really think that you should let the maintainers of this package decide when a PR is ready and can be merged. In my opinion, being a member of JuliaStats doesn't mean that you are able and should merge PRs in repos that you haven't contributed to and/or are not familiar with. For instance, there are many JuliaStats packages where I don't think it's appropriate for me to merge PRs. |
|
Clearly, we should not over-optimize this PR. But at the same time we should hold off merging PRs if reviewer comments that can be addressed without too much effort are not resolved. For instance, I think currently the following things are missing in this PR:
|
5 participants
Adds the functions logmeanexp, logvarexp, logstdexp