Open
Simplify logstatsexp reductions: length-aware iterators and direct normalization#1
logstatsexp reductions: length-aware iterators and direct normalization#1Conversation
Copilot
AI
changed the title
Retry PR #77: add logmeanexp, logvarexp, and logstdexp
Retry logstatsexp PR: add iterator support, remove avoidable allocations, and extend type coverage
Jun 3, 2026
Copilot created this pull request from a session on behalf of
cossio
June 3, 2026 18:56
View session
There was a problem hiding this comment.
Pull request overview
This PR introduces stable “log of exp-statistics” utilities to LogExpFunctions.jl, adding public APIs for log-mean/variance/std of exp(X) across both array reductions (dims) and non-array iterables, with accompanying tests and documentation wiring.
Changes:
- Add
logmeanexp,logvarexp, andlogstdexpto the public API (exports + module include). - Add a new implementation file for these functions and wire them into the docs index.
- Add a new test file covering array
dimsbehavior and some iterable inputs, plus test dependency updates.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/LogExpFunctions.jl |
Exports and includes the new log*exp statistics functions. |
src/logstatsexp.jl |
Implements logmeanexp / logvarexp / logstdexp for arrays and iterables. |
test/runtests.jl |
Adds the new logstatsexp.jl testset to the test suite. |
test/Project.toml |
Adds Statistics as a test dependency. |
test/logstatsexp.jl |
Adds tests for the new functionality (arrays + some iterators). |
docs/src/index.md |
Adds the new API entries to the documentation index. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Copilot
AI
changed the title
Retry logstatsexp PR: add iterator support, remove avoidable allocations, and extend type coverage
Expand logstatsexp tests to cover unresolved PR #77 review concerns
Jun 3, 2026
…ocstrings to say real numbers
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Addresses review feedback on the `dims != :` variance path: it no longer materializes a full-size `2 .* logsubexp.(X, logmean)` temporary before reducing. Instead `_LazyLogSqDev` is a read-only `AbstractArray` view of that broadcast, so `logsumexp(...; dims)` reduces it on the fly and only allocates the (smaller) reduced output. A bare `Broadcasted` can't be reduced along `dims` (no `axes(_, i)`), hence the thin wrapper. The elementwise term `2 * logsubexp(xᵢ, logmean)` is factored into `_logsqdev_term`, shared by the scalar single-pass loop and the lazy view. Also: - repair the "iterators" `@testset` whose closing `end` was dropped when the empty-array regression test was added (the file no longer parsed); - add an empty-array `logvarexp(Float64[])` regression test; - add allocation regression tests asserting the `dims` reductions allocate only the fixed-size reduced output, not an O(length(X)) temporary. Verified on Julia 1.10.11 and 1.12.6: full Pkg.test (incl. JET + Aqua) passes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A self-review of this branch surfaced several correctness bugs, all on the `dims` array paths. Fixes: - Revert the `_LazyLogSqDev` lazy-reduction wrapper back to the plain `logsumexp(2 .* logsubexp.(X, logmean); dims)`. The wrapper forwarded raw Cartesian indices to the wrapped `Broadcasted`, which silently returned WRONG variance/std for arrays with non-1-based axes (e.g. OffsetArrays — mean was correct, so mean and variance became mutually inconsistent), and threw a MethodError for abstract element types (`combine_eltypes` → `Any`). The factor of 2 cannot be pulled out of `logsumexp` (`log Σ exp(2y) ≠ 2 log Σ exp(y)`), and the materialized broadcast is correct and simpler; one temporary the size of the input is the same thing `Statistics.var(exp.(X); dims)` allocates. - Empty reduction with `corrected=true` no longer throws `DomainError` from `log(-1)`; `_finish_logvar` clamps the (corrected) count to ≥ 0 so an empty reduction yields `NaN`, matching `Statistics.var` and `logmeanexp`. - Empty along a non-reduced dimension no longer throws `DivideError` (0 ÷ 0); `_reduced_count` returns 0 when the reduced result is empty, giving an empty array of the reduced shape. - Complex arrays with `dims` now throw the clear "require real inputs" ArgumentError (the variance/std array methods accept `<:Number` and reject non-real eltype up front via `_require_real_array`) instead of a confusing MethodError; behavior now matches the no-`dims` path. Adds an "edge-case regressions" testset covering all of the above (OffsetArrays, abstract eltype, empty reduced/non-reduced dims, complex + dims). Removes the dims allocation test that asserted the (now reverted) lazy fusion. Verified on Julia 1.10.11 and 1.12.6. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`_centered_logsqdev` and `_logsumexp_and_count` were two near-identical hand-rolled iterate/accumulate loops differing only by a per-element transform. Replace both with a single `_logsumexp_count(f, X)` that returns `(logsumexp(f(xᵢ)), count)` in one pass — `logmeanexp` uses `f = identity`, `_centered_logsqdev` uses the centered-square term via `Base.Fix2`. Also factor the duplicated "require real inputs" message into `_throw_not_real` so the two validators can't drift. No behavior change. Verified on Julia 1.10.11 and 1.12.6: allocations, type-stability/inference, JET and Aqua all unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The generic logmeanexp(X) had a length-known branch that called logsumexp(X) (which probes isempty(X)) plus a separate length(X). For a single-use iterator that reports a length, the isempty probe consumed the first element, so the mean was computed over the remaining elements but divided by the full count — a silently wrong result (the variance path already materialized such iterators, so mean and variance were inconsistent). Always count during the single _logsumexp_count pass instead. This also drops the two-branch structure and the `_known_length` helper (counting requires a full pass regardless, so the length shortcut never saved work). The array method is unaffected (arrays dispatch to their own method). Adds a regression test with a single-use, length-reporting iterator. Verified on Julia 1.10.11 and 1.12.6 (allocations, inference, JET, Aqua green). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: David Müller-Widmann <devmotion@users.noreply.github.com>
Co-authored-by: David Müller-Widmann <devmotion@users.noreply.github.com>
Co-authored-by: David Müller-Widmann <devmotion@users.noreply.github.com>
Address review comments on JuliaStats#120 (no behavior change except empty array mean, see below): - Trim the verbose/editorializing header and helper comments; keep only the formula and the non-obvious rationale. - Merge the duplicated per-method docstrings into one docstring per function that also documents the array `dims` form. - Scalar `logmeanexp` now computes `lse - log(oftype(lse, n))` directly instead of going through the `_log_count` helper. - `logmeanexp(::AbstractArray; dims)` subtracts in place for the array case, avoiding a second temporary; empty-array reduction now returns `NaN` (matching `mean`) instead of throwing. - Use keyword-argument punning (`; dims`, `; corrected`) throughout. `_materialize` is kept (it lets re-iterable containers stay allocation-free while collecting single-use iterators for the variance's two passes). Tested on Julia 1.10 and 1.12: full logstatsexp testset, JET, and Aqua pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
01d92c8 to
691e34e
Compare
…e output
Mirror the structure of `src/logsumexp.jl`:
- Add exported `logmeanexp!(out, X)` and `logvarexp!(out, X; corrected)` that
reduce over the singleton dimensions of `out` and write the result there,
reusing `logsumexp!` (so they allocate only the same `Tuple{FT,FT}` scratch
`logsumexp!` itself uses — no input-sized temporary).
- `logmeanexp`/`logvarexp`/`logstdexp`/`logmeanexp_and_log*` with `dims` now
allocate the (output-sized) result and delegate to the in-place versions,
instead of materializing the full `2 .* logsubexp.(X, logmean)` array. The
`dims` variance went from O(n) to O(output) allocations.
- The centered terms are reduced lazily through a small `_LogSqDev` array (a
plain `AbstractArray`, not a `Broadcasted`, so `reduce`/`reducedim!` honor
offset axes and a concrete element type).
Tests: add an in-place correctness testset (dense, OffsetArray, abstract
eltype, complex rejection) and extend the allocation tests to assert the
`dims` and in-place paths do not scale with the input size.
Verified on Julia 1.10 and 1.12: full logstatsexp testset, JET, and Aqua pass.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Drop `logmeanexp_and_logvarexp` and `logmeanexp_and_logstdexp` (both the iterator and `dims` array methods), their exports, and their docs entries. They computed two statistics at once, which made them harder to review for little benefit over calling `logmeanexp` and `logvarexp`/`logstdexp`. - Reimplement the iterator `logvarexp(X)` standalone — it previously delegated to `logmeanexp_and_logvarexp`. It now materializes once, takes the mean, and reduces the centered squared deviations directly. `_materialize` is kept so single-use iterators survive the variance's two passes. - Remove the now-unused `_centered_logvar` helper (it only served the combined array method); `logvarexp!`/`_LogSqDev` are unchanged. - Prune the combined-function testsets and references. Verified on Julia 1.10 and 1.12: full logstatsexp testset, JET, and Aqua pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Hoist `oftype` to wrap the full expression in the scalar logmeanexp / logvarexp reductions, and add a `return types` testset asserting Float32->Float32, Float64->Float64, and Int->Float64 across the scalar, dims, dims=:, iterator, and in-place paths (via @inferred). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0161bYmHCny8nuA4agW2KnJ1
Tighten the multi-line explanatory comments in test/logstatsexp.jl while keeping the rationale each one records. Comment-only; no test logic changed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0161bYmHCny8nuA4agW2KnJ1
The previous commit accidentally added two local Claude Code worktree directories as gitlinks (mode 160000); remove them and ignore .claude/. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0161bYmHCny8nuA4agW2KnJ1
Keep .claude out of the tracked .gitignore; it is ignored locally via .git/info/exclude instead (per-clone, not committed). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0161bYmHCny8nuA4agW2KnJ1
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Simplify the
logstatsexpreductions and add the combined mean+variance / mean+std functions, while keeping the numerically-stable behavior and being allocation-conscious.Functions
logmeanexp,logvarexp,logstdexp—logof the mean / variance / std ofexp.(X).logmeanexp_and_logvarexp,logmeanexp_and_logstdexp— compute both at once, reusing the mean to center the variance (cheaper than two separate calls).All accept either a general iterator or an
AbstractArraywith adimskeyword, and acorrectedkeyword for the variance/std.Numerical stability
The variance uses the centered formula
i.e. the log of
∑ᵢ (exp(xᵢ) - mean)²divided by the count. Centering is essential: the raw-second-moment alternativelogsubexp(∑exp(2xᵢ), (∑exp(xᵢ))²/n)cancels catastrophically when the variance is small relative to the mean (and can even overflow toInffor nearly-equal inputs). Because the variance needs the mean first,logmeanexp_and_logvarexpis the primitive andlogvarexp/logstdexp/logmeanexp_and_logstdexpare derived from it. Correctness on hard cases (tight clusters, over/underflowing magnitudes, huge dynamic range) is checked againstBigFloatreferences in the tests.Iterator / reduction strategy
logmeanexp) is a single pass that counts the elements as it goes, so an iterator is traversed exactly once (single-use iterators are handled correctly). Empty inputs throw.IteratorSize(Iterators.StatefulreportsHasLengthon some Julia versions yet is single-use), so a generic iterator iscollected once; known re-iterable containers (AbstractArray,Tuple,NamedTuple,AbstractRange) are traversed in place.Allocations
dims = :) variance accumulates in a single pass (_centered_logsqdev) with no intermediate array.dimsvariance reduces2 .* logsubexp.(X, logmean)withlogsumexp(...; dims). That allocates one temporary the size of the input — the same intermediateStatistics.var(exp.(X); dims)itself forms. (An earlier lazy-reduction variant that avoided this temporary was dropped: it returned silently wrong results for arrays with non-1-based axes and threw for abstract element types — simplicity and correctness won.)log(n)in the input's own (real) floating-point type, soFloat32staysFloat32(no silentFloat64promotion).Edge cases
var/stdsemantics are matched on the awkward inputs: an empty reduction yieldsNaN(not an error) for everycorrected, an empty non-reduced dimension yields an empty result, non-1-based axes (OffsetArrays) match the 1-based result, abstract element types work, and complex inputs are rejected with a clearArgumentErroron every variance/std path.Tests
Cover arrays and iterators (incl. one-shot
Statefuland a single-use length-reporting iterator),dimsand promotion, the combined functions, type-stability /@inferred, allocation non-scaling for the full reductions, the edge cases above, empty-input errors, real-input enforcement for variance/std, and theBigFloatnumerical-robustness suite. Verified on Julia 1.10 and 1.12 (fullPkg.test, including JET and Aqua).