Skip to content

[MRG] Fix swapped div_to_product arguments in fused_unbalanced_across_spaces_cost - #855

Open
itzzdev09 wants to merge 3 commits into
PythonOT:masterfrom
itzzdev09:fix-ucoot-div-to-product-args
Open

itzzdev09 wants to merge 3 commits into
PythonOT:masterfrom
itzzdev09:fix-ucoot-div-to-product-args

Conversation

@itzzdev09

@itzzdev09 itzzdev09 commented Sep 14, 2026

Copy link
Copy Markdown

Types of changes

  • Bug fix

Motivation and context / Related issue

Fixes #854.

div_to_product takes (pi, a, b, pi1=None, pi2=None, ...), but both calls in fused_unbalanced_across_spaces_cost (the reg_type="independent" / UCOOT branch) passed (pi, pi1, pi2, a, b, ...). The entropic terms were therefore computed with the plan marginals as the reference measures a, b and the reference measures px, py as the marginals pi1, pi2. For divergence="l2", where pi1/pi2 are ignored, the cost was taken against the product of the plan's own marginals instead of px ⊗ py.

This swaps the arguments back into the documented order for both the sample and the feature coupling.

How has this been tested (if it applies)

Added test_fused_unbalanced_across_spaces_cost_independent_reg (KL and L2): it checks that the regularisation part of the cost equals eps_samp * div_to_product(pi_samp, px_samp, py_samp) + eps_feat * div_to_product(pi_feat, px_feat, py_feat). The test fails on current master for both divergences and passes with the fix. test/gromov (217 passed) and test/test_ucoot.py (133 passed) pass locally with the numpy backend.

AI disclosure: this fix and test were made with Claude Code under my supervision; I checked the argument order against the div_to_product signature and ran the tests locally.

PR checklist

  • I have read the CONTRIBUTING document.
  • The documentation is up-to-date with the changes I made (check build artifacts).
  • All tests passed, and additional code has been covered with new tests.
  • I have added the PR and Issue fix to the RELEASES.md file.

🤖 Generated with Claude Code

…s_cost

div_to_product(pi, a, b, pi1, pi2) was called as (pi, pi1, pi2, a, b),
so with reg_type="independent" (UCOOT) the entropic terms used the plan
marginals as reference measures and the reference measures as marginals.

Fixes PythonOT#854

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@rflamary
rflamary requested a review from 6Ulm September 15, 2026 08:15

@6Ulm 6Ulm left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

All good to me.

Nice catch! Just a small note: this bug concerns only the regularisation term. In practice, epsilon is 0 or very small, so this bug has limited impact on the final results (OT plans and costs).

@codecov

codecov Bot commented Sep 15, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.85%. Comparing base (3073aa0) to head (46b3042).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #855   +/-   ##
=======================================
  Coverage   96.85%   96.85%           
=======================================
  Files         128      128           
  Lines       26161    26181   +20     
=======================================
+ Hits        25337    25357   +20     
  Misses        824      824           
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Typo in ot.gromov._utils fused_unbalanced_across_spaces_cost regularization cost

3 participants