Conversation
Backend.logsumexp is declared as logsumexp(self, a, axis=None, keepdims=False) and documented to follow the scipy.special.logsumexp API. NumPy, Jax and Torch honour that signature, but CupyBackend and TensorflowBackend were declared without keepdims and raised a TypeError if it was passed. CupyBackend.logsumexp also reimplemented a 2021 SciPy snippet by hand; it now delegates to cupyx.scipy.special.logsumexp, which matches the hand-rolled all-(-inf) handling exactly (verified against the current cupy source) and adds keepdims support in one call. TensorflowBackend.logsumexp now forwards keepdims to tf.math.reduce_logsumexp, which already supports it. No current caller passes keepdims, so this changes no behaviour for existing callers. Extends test_func_backends in test/test_backend.py with axis=0, axis=1, keepdims=True and an all-(-inf)-row case, verified on numpy/jax/torch/tf; the Cupy path is unverified by CI (no Cupy/GPU available here). Fixes PythonOT#867 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
convolutional_barycenter2d and convolutional_barycenter2d_debiased were hardcoded to 2D images, using a separate width/height convolution operator (_get_convol_img_fn) and, in log-domain, a Python loop over histograms with in-place writes that jax and tf could not trace. Replace _get_convol_img_fn with a general _SeparableKernel: it applies one 1D Gaussian factor per grid axis via a dense matmul (exp-domain) or a stabilized log-matmul-exp (log-domain), treating any leading axes as batch. This works unchanged for 1D signals, 2D images, 3D volumes, or higher, and processes all histograms in a batch instead of looping, which removes the mutable state that blocked jax/tf under method="sinkhorn_log". Add the N-D public API: convolutional_grid_barycenter and convolutional_grid_barycenter_debiased, for A of shape (n_hists, *grid_shape). convolutional_barycenter2d and convolutional_barycenter2d_debiased become thin wrappers that check A.ndim == 3 and delegate; their signatures, defaults, docstrings and return contract are unchanged, and they are not deprecated. The log=True return contract keeps its existing asymmetry (only the exp-domain solvers populate log["U"]/log["V"]), now applied consistently across all four grid functions. Along the way, fix bugs found while touching this code: array-method calls (.std()/.sum()) instead of nx.std()/nx.sum(), an in-place log_bar += rebind that is invalid under jax/tf, and a dead `F` variable from an unused triple-zeros allocation. Verified: - _SeparableKernel matches the removed _get_convol_img_fn to ~1.8e-15 (exp-domain) and ~4.4e-16 (log-domain, fast) / exactly 0.0 against a module-private exact-logsumexp test helper, on random (4, 9, 7) input. - The fast, shifted log-matmul-exp agrees with the exact per-axis logsumexp reduction to ~1e-15 for reg >= 1e-3 on a moderate grid; for an exact delta input at reg <= 1e-4 it underflows to -inf where the exact reduction stays finite (regression-tested as a known, documented limitation of method="sinkhorn_log"). - jax and tf now genuinely pass sinkhorn_log (previously NotImplementedError), verified in a separate venv with tf installed. - On the 4-image, reg=0.004 example: sinkhorn 230->30ms (7.7x), sinkhorn_log 8653->133ms (65x), debiased sinkhorn 350->48ms (7.3x), debiased sinkhorn_log 12924->235ms (55x), with old-vs-new output agreement to ~1e-17 (see local_sandbox/bench_convolutional_barycenter.py, not part of this commit). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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.
Summary
Pure refactor of
ot/bregman/_convolutional.py: no new solver, no new maths.Backend.logsumexp(Cupy/Tensorflow didn't acceptkeepdims, unlike NumPy/Jax/Torch) — a prerequisite for writing backend-agnostic log-domain code in commit 2. See Backend.logsumexp: keepdims missing from Cupy and Tensorflow backends; Cupy reimplements cupyx.scipy.special.logsumexp #867.convolutional_barycenter2d/convolutional_barycenter2d_debiasedfrom hardcoded 2D images to arbitrary-dimension regular grids (1D signals, 3D volumes, ...), via a new_SeparableKernelapplied through densematmul(exp-domain) or a stabilized log-matmul-exp (log-domain), and vectorizes the log-domain solvers across histograms instead of looping with in-place writes.convolutional_barycenter2d{,_debiased}become thinA.ndim == 3wrappers with unchanged signatures, defaults, docstrings and return contract; they are not deprecated.Motivation
This is related to #862 (faster exact EMD on shared Cartesian grids). That discussion is about exact, network-flow-based OT on grids; this PR is a step toward the entropic/approximate side of the same problem space: generalizing the convolutional-kernel machinery to N-D grids is the natural prerequisite for adding the convolutional Wasserstein distance (Solomon et al. 2015, Algorithm 1) as a Sinkhorn-based alternative for OT/barycenters on d-dimensional grids — a follow-up PR builds on top of what's here. This PR does not implement that distance function itself.
Why
matmuland not a native 1-D convolutionThe Gaussian kernel on a grid axis is Toeplitz, so applying it via
K @ xis mathematically equivalent to a (zero-padded) 1-D convolution — verified to agree to ~3e-15 on a small example — but a native convolution isn't used here, for two reasons:matmulgoes through BLAS GEMM. Measured (Torch 2.14, CPU, float64, 5 images,n=512,reg=4e-3): ~4.5 ms formatmulvs. ~220 ms for a depthwiseconv2dwith a 4-sigma-truncated kernel.examples/barycenters/plot_convolutional_barycenter.py, 64x64 images,reg=0.004, so σ≈2.8 px), truncating the kernel below 12σ makes the Sinkhorn iteration diverge to NaN. Becauseregis a physical quantity, σ in pixels grows with the grid for fixedreg(~2.8 px at n=64 vs. ~22.9 px at n=512), so truncation could only ever be a constant-factor saving, never an asymptotic one.This is documented as a
.. note::on_SeparableKernelso it isn't relitigated in review.Correctness verification
_get_convol_img_fn, the new_SeparableKernelwas checked against it on random(4, 9, 7)input: exp-domain max abs diff ~1.8e-15; log-domain (fast, shifted) ~4.4e-16; log-domain against a module-private exact-logsumexp test helper (_exact_separable_log_apply, not part of the public API) exactly0.0._SeparableKernelin log-domain is a numerics change relative to the exact per-axislogsumexpreduction (not bit-identical). Regression-tested to agree to ~1e-9 (measured ~1e-15/1e-16) forreg >= 1e-3on a moderate grid. Known, tested limitation: for an exact delta input atreg <= 1e-4it underflows to-infwhere the exact reduction stays finite.method="sinkhorn_log"previously raisedNotImplementedErrorfor Jax/Tensorflow because of in-place writes (log_KU[k] = ...) in a per-histogram Python loop. Vectorizing across histograms removes the need for that, and both backends now genuinely pass (verified in a separate venv withtensorflowinstalled, since it isn't in this session's default env).test/test_bregman.pypass unchanged, except the jax/TF guards described above. No test tolerance needed adjusting.log=Truereturn contract._convolutional_barycenter2dputsU/Vin the log dict; the_logvariant does not. This asymmetry is preserved and now applied consistently across all four new N-D functions.Benchmark
4 example images (
examples/barycenters/plot_convolutional_barycenter.py),reg=0.004,numItermax=1000:sinkhornsinkhorn_logsinkhorn(debiased)sinkhorn_log(debiased)The large
sinkhorn_logspeedup comes from removing the per-histogram Python loop; thesinkhornspeedup comes frommatmul-based batched kernel application replacingeinsum. Old-vs-new agreement is at floating-point noise level.Benchmark script (not part of this PR's tracked files — kept locally under the gitignored
local_sandbox/)New public API
ot.bregman.convolutional_grid_barycenter(A, reg, ...)—Aof shape(n_hists, *grid_shape), any number of grid dimensions.ot.bregman.convolutional_grid_barycenter_debiased(A, reg, ...)— debiased variant.convolutional_barycenter2d/convolutional_barycenter2d_debiasedare unchanged thin wrappers (A.ndim == 3check, then delegate).Tests
(3, 40)) and 3D ((3, 12, 13, 14)) barycenters, both methods, debiased and not, parametrized over backends.convolutional_barycenter2d{,_debiased}vsconvolutional_grid_barycenter{,_debiased}equivalence forA.ndim == 3, andValueErroron wrongndim.sinkhorn_logguards intest/test_bregman.pynow exercise the real solver instead of expectingNotImplementedError.test/test_backend.pyfor theBackend.logsumexpfix (axis,keepdims, all--infrow).Out of scope / deferred to follow-ups
nx.convolve1dbackend primitive (see "Whymatmul" above).spacing/extentargument for non-unit grids, or any change to thelinspace(0, 1, n)grid convention.🤖 Generated with Claude Code