Skip to content

Simplify logstatsexp reductions: length-aware iterators and direct normalization#1

Open
cossio with Copilot wants to merge 40 commits into
masterfrom
copilot/retry-pr-77
Open

Simplify logstatsexp reductions: length-aware iterators and direct normalization#1
cossio with Copilot wants to merge 40 commits into
masterfrom
copilot/retry-pr-77

Conversation

Copilot AI commented Jun 3, 2026

Copy link
Copy Markdown

Simplify the logstatsexp reductions and add the combined mean+variance / mean+std functions, while keeping the numerically-stable behavior and being allocation-conscious.

Functions

  • logmeanexp, logvarexp, logstdexplog of the mean / variance / std of exp.(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 AbstractArray with a dims keyword, and a corrected keyword for the variance/std.

Numerical stability

The variance uses the centered formula

log var = logsumexp(2 * logsubexp(xᵢ, logmean)) - log(n - corrected)

i.e. the log of ∑ᵢ (exp(xᵢ) - mean)² divided by the count. Centering is essential: the raw-second-moment alternative logsubexp(∑exp(2xᵢ), (∑exp(xᵢ))²/n) cancels catastrophically when the variance is small relative to the mean (and can even overflow to Inf for nearly-equal inputs). Because the variance needs the mean first, logmeanexp_and_logvarexp is the primitive and logvarexp / logstdexp / logmeanexp_and_logstdexp are derived from it. Correctness on hard cases (tight clusters, over/underflowing magnitudes, huge dynamic range) is checked against BigFloat references in the tests.

Iterator / reduction strategy

  • Mean (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.
  • Variance is centered, so it needs two traversals (mean, then deviations). Re-iterability cannot be detected from IteratorSize (Iterators.Stateful reports HasLength on some Julia versions yet is single-use), so a generic iterator is collected once; known re-iterable containers (AbstractArray, Tuple, NamedTuple, AbstractRange) are traversed in place.

Allocations

  • The scalar (dims = :) variance accumulates in a single pass (_centered_logsqdev) with no intermediate array.
  • The dims variance reduces 2 .* logsubexp.(X, logmean) with logsumexp(...; dims). That allocates one temporary the size of the input — the same intermediate Statistics.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.)
  • Normalization uses log(n) in the input's own (real) floating-point type, so Float32 stays Float32 (no silent Float64 promotion).

Edge cases

var/std semantics are matched on the awkward inputs: an empty reduction yields NaN (not an error) for every corrected, 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 clear ArgumentError on every variance/std path.

Tests

Cover arrays and iterators (incl. one-shot Stateful and a single-use length-reporting iterator), dims and 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 the BigFloat numerical-robustness suite. Verified on Julia 1.10 and 1.12 (full Pkg.test, including JET and Aqua).

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 finished work on behalf of cossio June 3, 2026 18:56
Copilot AI requested a review from cossio June 3, 2026 18:56
@cossio cossio marked this pull request as ready for review June 3, 2026 19:13
Copilot AI review requested due to automatic review settings June 3, 2026 19:13

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, and logstdexp to 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 dims behavior 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.

Comment thread src/logstatsexp.jl Outdated
Comment thread src/logstatsexp.jl Outdated
Comment thread src/logstatsexp.jl Outdated
Comment thread test/logstatsexp.jl Outdated
Comment thread src/logstatsexp.jl Outdated
Comment thread src/logstatsexp.jl Outdated
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
Copilot finished work on behalf of cossio June 3, 2026 19:29

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Comment thread src/logstatsexp.jl Outdated
Comment thread src/logstatsexp.jl Outdated
Comment thread src/logstatsexp.jl Outdated
Comment thread src/LogExpFunctions.jl

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Comment thread src/logstatsexp.jl Outdated
Comment thread src/logstatsexp.jl Outdated
Comment thread src/LogExpFunctions.jl
cossio and others added 10 commits June 4, 2026 21:41
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>
@cossio cossio force-pushed the copilot/retry-pr-77 branch from 01d92c8 to 691e34e Compare June 8, 2026 18:15
cossio and others added 19 commits June 8, 2026 21:47
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants