[MRG] entropic_partial_wasserstein: add method='sinkhorn_log' stable log-domain solver (rescue of #724)#811
[MRG] entropic_partial_wasserstein: add method='sinkhorn_log' stable log-domain solver (rescue of #724)#811hinanohart wants to merge 2 commits into
method='sinkhorn_log' stable log-domain solver (rescue of #724)#811Conversation
… (rescue of PythonOT#724) Re-applies the function from PR PythonOT#724 by wzm2256 on top of current master (the original PR is stuck at CONFLICTING since 2025-09; this takes the additive parts and skips the obsolete merges through the March-2025 single-file layout). Subject is [WIP] because the original PR is also [WIP] and maintainer review is still required. Changes vs master: ot/partial/partial_solvers.py + entropic_partial_wasserstein_logscale (function body verbatim from PR PythonOT#724 modulo: (a) duplicate sphinx label removed to avoid build failure, (b) print warning -> warnings.warn(stacklevel=2) for convention). ot/partial/__init__.py + entropic_partial_wasserstein_logscale export test/test_partial.py + test_entropic_partial_wasserstein_logscale_matches_old_at_large_reg (machine-precision agreement at reg in {10.0, 1.0}: atol=1e-10) + test_entropic_partial_wasserstein_logscale_no_nan_at_small_reg (parametrised over reg in {0.1, 0.05, 0.01, 5e-3, 1e-3, 5e-4}) + test_entropic_partial_wasserstein_logscale_approaches_exact_at_small_reg (plan-cost gap vs exact partial OT at reg=1e-3) + test_entropic_partial_wasserstein_logscale_log_dict + test_entropic_partial_wasserstein_logscale_input_validation examples/unbalanced-partial/plot_entropic_partial_wasserstein_logscale.py + Sphinx-Gallery example reproducing issue PythonOT#723 + the fix (MPLBACKEND=Agg-safe; narrative softened to acknowledge BLAS/platform-dependent underflow boundary). docs/source/user_guide.rst + one-paragraph mention next to entropic_partial_wasserstein. RELEASES.md + entry under 0.9.7.dev0 (phrased as "mitigated via new log-domain variant", not "fixed", since the standard solver itself is unchanged). Verified locally on master at 41a4d57: pytest test/ -> 1939 passed, 97 skipped, 6 xfailed (no regressions). pytest test/test_partial.py -> 19 passed (8 originals + 11 new parametrised cases for the logscale function). Example script runs end-to-end with MPLBACKEND=Agg. The new function agrees with the standard solver at reg >= 1.0 to ~1e-18 absolute (atol=1e-10 in tests is conservative) and stays finite at reg down to 5e-4 on a 50x50 cost-scale-~50 problem (the exact failure mode of issue PythonOT#723); std solver returns NaN at reg ~ 0.05-0.01 on the same problem. References Issue PythonOT#723. Maintainer review needed before merge — author attribution to wzm2256 retained via Co-authored-by trailer. Co-authored-by: wzm2256 <wzm2256@qq.com>
|
Hello @hinanohart, thanks for this PR, we are OK with emrging this new rescue, we really appreciate your new contributions. I have fixed the failing tests and doc in master so I would appreciate if you merge master to this PR so that we can do a proper code review when all tests pass. |
|
Hello @hinanohart , could you merge and resolve conflicts please? I really don't want to rescue another rescue ;) BTW I had alook at your code and I think it would be better to do like we did in ot.sinkhorn an add a method parameter that allows to choose between sinkhorn and sinkhorn_log to chose classical or log solver. That is making the classical partial solver a wrapper over the two solvers. Could you do that so that users can now use your new algorithm by just setting a parameter? |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #811 +/- ##
==========================================
- Coverage 96.80% 96.79% -0.02%
==========================================
Files 118 118
Lines 23721 23877 +156
==========================================
+ Hits 22963 23111 +148
- Misses 758 766 +8 🚀 New features to boost your workflow:
|
Resolves the RELEASES.md conflict from merging current master into the
rescue-pr-724 branch (the only conflict was in the 0.9.7.dev0
"New features" list; master had advanced with the ot.sgot and ARM-wheel
entries).
Following the ot.sinkhorn convention, entropic_partial_wasserstein now
takes a `method` parameter and becomes a thin wrapper dispatching between
two solvers:
- method="sinkhorn" (default) classical multiplicative-domain
solver, extracted unchanged into the private
_entropic_partial_wasserstein_sinkhorn
- method="sinkhorn_log" numerically stable log-domain solver
entropic_partial_wasserstein_logscale
The default path reproduces the previous behaviour byte-for-byte, so
existing callers (including the ot.gromov partial solvers, which pass
a, b, M, reg, m positionally) are unaffected. Users opt into the PythonOT#723
NaN fix by setting a single parameter.
Docs, the gallery example and RELEASES.md updated to describe the
parameter; tests added for the dispatch (default == sinkhorn,
method="sinkhorn_log" == logscale, case-insensitivity, log-dict
forwarding, invalid-method ValueError).
Co-authored-by: wzm2256 <wzm2256@qq.com>
|
Done on both counts:
Docs, the gallery example, |
method='sinkhorn_log' stable log-domain solver (rescue of #724)
|
All CI is green on the merged branch — |
Context
This is a rescue follow-up to PR #724 (and issue #723), both opened by @wzm2256 in March 2025. The original PR adds a numerically-stable
entropic_partial_wasserstein_logscaleto fix the NaN regime that #723 documents, but has been stuckCONFLICTINGsince 2025-09 — the author wrote in the PR body that they could not (a) build the docs locally (sphinx_rtd_thememissing) or (b) write pytest cases, and asked for help.I hit the same NaN regime as a downstream user of
entropic_partial_wasserstein, so this PR re-applies @wzm2256's function on top of currentmasterand supplies the missing pieces.Why a new PR rather than pushing to #724's branch
When PR #724 was opened,
ot/partial.pywas a single file. The maintainers later split it into theot/partial/package, so the diff between #724's branch and currentmasteris now +2k / −13k lines of mostly unrelated movement. Re-applying just the new function on the new layout is cleaner than fighting that rebase.If maintainers prefer to keep #724 as the canonical PR, I'm happy to close this and instead push these changes to that branch (write access permitting).
What's added (additive only; nothing removed)
ot/partial/partial_solvers.py—entropic_partial_wasserstein_logscalefunction, identical algorithm to PR [WIP] Add a stablized function entropic_partial_wasserstein_logscale #724 re-applied on the current layout.ot/partial/__init__.py— export +__all__entry.test/test_partial.py— 5 new test functions / 11 parametrised cases:[0.1, 0.05, 0.01, 5e-3, 1e-3, 5e-4]— reproduces issue entropic_partial_wasserstein not stable #723's failure mode and verifies the logscale solver stays finite.atol=1e-10, rtol=1e-10) across[1.0, 10.0].approaches_exact_at_small_reg— checks the plan-cost vs. exact partial-OT gap atreg=1e-3(not just NaN-freeness; verifies mathematical correctness).examples/unbalanced-partial/plot_entropic_partial_wasserstein_logscale.py— Sphinx-Gallery example reproducing entropic_partial_wasserstein not stable #723's failure and showing the fix. Adapted from @wzm2256'scompare_logscale_POT.py.docs/source/user_guide.rst— one-paragraph mention next toentropic_partial_wasserstein.RELEASES.md— one-line entry under0.9.7.dev0, phrased as mitigation (the standard solver itself is unchanged; callers opt into the log-domain variant).Local verification
pytest test/test_partial.py -q→ 19 passed (8 originals + 11 new parametrised cases).pytest test/(full suite) → 1939 passed, 97 skipped, 6 xfailed — no regression outsidepartial.MPLBACKEND=Agg. Standard solver returns NaN atreg ∈ {0.05, 0.01}on the 50×50 cost-scale-50 problem; logscale solver stays finite over the full sweep.git diff master -- ot/partial/partial_solvers.pyshowed exactly one hunk: the new function block. (The follow-up below adds themethod-dispatch refactor on top of that.)Update (2026-06-01) — merged
master+methodparameterAddressing @rflamary's review:
masterand resolved the conflict. The only conflict was in theRELEASES.md0.9.7.dev0 "New features" list (theot.sgot/ ARM-wheel entries vs. the partial entry); the PR is nowMERGEABLE.methodparameter following theot.sinkhornconvention.entropic_partial_wasserstein(..., method=...)is now a thin wrapper:method='sinkhorn'(default) runs the classical multiplicative-domain solver andmethod='sinkhorn_log'the stable log-domain one. The classical iteration moved verbatim into a private_entropic_partial_wasserstein_sinkhorn, so the default path is byte-for-byte unchanged and the three internalot.gromov/ot.partialcallers (which passa, b, M, reg, mpositionally) are unaffected. Docs, the gallery example,RELEASES.mdand tests were updated to cover the dispatch.Co-authorship
Credit for the actual numerical work belongs to @wzm2256, and the commit carries a
Co-authored-by: wzm2256 <wzm2256@qq.com>trailer to reflect that (matching this repo's convention). If you'd rather I drop the trailer or keep the acknowledgement to the PR description only, just say so.cc @rflamary @cedricvincentcuaz — you both did
master-merges on #724 earlier this year so it looked like a "we want this in" signal. Happy to do whatever's least friction here: this PR can be the canonical one, or I can close it and push to #724's branch.The
[WIP]prefix matches the original PR's convention and signals this is open for re-titling to[MRG]once you've had a look.