Skip to content

Sinkhorn loss implementatin#809

Open
guglielmopadula wants to merge 5 commits into
mathLab:0.3from
guglielmopadula:0.3-reg
Open

Sinkhorn loss implementatin#809
guglielmopadula wants to merge 5 commits into
mathLab:0.3from
guglielmopadula:0.3-reg

Conversation

@guglielmopadula

Copy link
Copy Markdown
Member

Description

This PR fixes #808 by adding a Sinkhorn loss implementation (the loss init file is modified, a new class, SinkhornLoss, is created along with the necessary tests).

Checklist

  • Code follows the project’s Code Style Guidelines
  • Tests have been added or updated
  • Documentation has been updated if necessary
  • Pull request is linked to an open issue

@guglielmopadula guglielmopadula requested review from a team and dario-coscia as code owners June 17, 2026 10:22
@GiovanniCanali GiovanniCanali added enhancement New feature or request pr-to-fix Label for PR that needs modification 0.3 Related to 0.3 release labels Jun 18, 2026
@GiovanniCanali GiovanniCanali linked an issue Jun 18, 2026 that may be closed by this pull request

@GiovanniCanali GiovanniCanali 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.

Thank you @guglielmopadula for the valuable contribution. The code and overall logic look good, only a few minor comments need to be addressed before merging.

Comment thread docs/source/_rst/loss/sinkhorn_loss.rst Outdated
Comment thread docs/source/_rst/loss/sinkhorn_loss.rst
Comment thread pina/_src/loss/sinkhorn_loss.py Outdated
Comment thread pina/_src/loss/sinkhorn_loss.py
Comment thread pina/_src/loss/sinkhorn_loss.py Outdated
Comment thread pina/_src/loss/sinkhorn_loss.py Outdated
Comment thread tests/test_loss/test_sinkhorn_loss.py Outdated
Comment thread tests/test_loss/test_sinkhorn_loss.py Outdated
Comment thread tests/test_loss/test_sinkhorn_loss.py Outdated
log_b - torch.logsumexp((f.unsqueeze(1) - C) / self.eps, dim=0)
)

loss = (a * f).sum() + (b * g).sum()

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.

If I understand correctly, this loss can take negative values. Please confirm whether this is expected behavior; otherwise, adjust the implementation accordingly.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The loss can indeed take negative values.

@guglielmopadula

Copy link
Copy Markdown
Member Author

I have fixed the code according to instructions. Thank you!

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

Labels

0.3 Related to 0.3 release enhancement New feature or request pr-to-fix Label for PR that needs modification

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add a Sinkhorn loss implementation

3 participants