logstatsexp#77
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:
|
|
I am closing this because of no activity. If there is interest, please bump here and I will reopen. That said, a fresh start would be preferred, addressing all the concerns here. |
|
@tpapp @devmotion Retrying at #120 |
…ogstdexp Rework the log-mean/var/std-exp reductions around a single primitive (`logsumexp(X)`, `logsumexp(2X)`, count), addressing review feedback in JuliaStats#120 and JuliaStats#77: - Use a single pass over general iterators (works for one-shot iterators such as `Iterators.Stateful`); take the count from `length` for arrays instead of accumulating it, as suggested by @tpapp. - Drop the explicit empty/`NaN` special-casing: for a single element the arithmetic already yields `NaN` (`-Inf - log(0)`), per @tpapp's note. - Remove the `logmean` keyword and the parallel array/iterator variance helpers; everything now flows through `_logvar`/`_logmoments`. - No promotion of `Float32` inputs; full reductions allocate nothing. Add `logmeanexp_and_logvarexp` and `logmeanexp_and_logstdexp` (the `mean_and_var` / `mean_and_std` analogues, also suggested by @tpapp), computing both statistics in a single pass. Tests: extend coverage with the new functions, allocation checks (zero allocations for full reductions over arrays/iterators), and type-stability/`@inferred` checks across `Float32`/`Float64`, `dims`, and `corrected`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
6 participants
Adds the functions logmeanexp, logvarexp, logstdexp