Conversation
…onOT#458) The 2D convolutional barycenters build K = exp(-(x-y)**2 / reg) on a unit grid. For small reg the distant entries underflow, so mass can no longer be moved across the image and the barycenter collapses towards the arithmetic mean of the inputs. That reads as an over-diffuse result rather than a failure, which is what PythonOT#458 reports. Measured on two Gaussians of equal width separated by 0.15, comparing the recovered width against the true one: the default solver is 236% off at reg=5e-4, 182% at 1e-3, 20% at 2e-3 and correct from 4e-3 up, while method='sinkhorn_log' is exact throughout. It is not a convergence problem, 200000 iterations at stopThr=1e-12 return the same wrong answer, and it is not the debiasing, which reproduces the barycenter of {mu, mu} to 4e-4. Sinkhorn forms products and ratios of kernel entries, so only about half the exponent range is usable. The warning triggers when the smallest kernel exponent falls below log(tiny)/2, which lands where the measured error stops being negligible. test_convolutional_barycenter_non_square used reg=1e-3, which underflows on any grid. Its uniform input needs no transport so the assertion held, but the kernel was degenerate; it now uses 1e-2 and exercises the same code path.
Review of the previous commit: - it converted the whole width x width exponent matrix with nx.to_numpy purely to read a dtype, which copies the array and forces a device sync on GPU backends, once per barycenter call; - it compared two backend scalars with the builtin min, relying on __lt__ returning a Python bool across all five backends; - the reduction was unnecessary. The grid is always linspace(0, 1, n), so the most negative exponent is -1 / reg whatever the image size. The check now derives the exponent from reg alone and reads the dtype from a single-element array. stacklevel was 3, which attributed the warning to ot/bregman/_convolutional.py rather than to the caller. Measured against the real call stack: 4 lands on the user's call site.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #873 +/- ##
=======================================
Coverage 96.86% 96.87%
=======================================
Files 128 128
Lines 26304 26345 +41
=======================================
+ Hits 25480 25521 +41
Misses 824 824 🚀 New features to boost your workflow:
|
This branch has not been deployed
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
1 participant
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.
Types of changes
Bug fix (non-breaking change which fixes an issue).
Motivation and context / Related issue
Closes #458.
The 2D convolutional barycenters build
K = exp(-(x-y)**2 / reg)on a unit grid. At smallregthe distant entries underflow, mass can no longer cross the image, and the barycenter collapses towards the arithmetic mean of the inputs — which reads as over-diffuse rather than as a failure.Two things it is not, both raised in the thread: not the debiasing (the barycenter of
{mu, mu}comes back asmuto L1 4e-4 at everyreg), and not slow convergence (200000 iterations atstopThr=1e-12give the same wrong answer as 10000 at1e-3).Recovered width for two Gaussians of width 0.06 separated by 0.15:
sinkhorn_logAt
reg=1e-3the width is 0.169 ~= sqrt(0.15^2 + 0.06^2): the inputs never merged.Sinkhorn forms products and ratios of kernel entries, so only about half the exponent range is usable. The warning triggers when the smallest kernel exponent falls below
log(tiny)/2, which is where the error stops being negligible, and points atmethod='sinkhorn_log'.test_convolutional_barycenter_non_squareusedreg=1e-3, which underflows on any grid; its uniform input needs no transport so the assertion held. Now1e-2, same code path. Happy to revert if you prefer the old value.How has this been tested (if it applies)
New test asserts the warning fires for both solvers and stays silent for a usable kernel and for
sinkhorn_log; verified to fail on master. A second pins the equal-width property from Janati et al. 2020. Full suite 2692 passed, 63 skipped, warning fires nowhere else. pre-commit clean.PR checklist