Skip to content

Fix incorrect use of optimal transport params in Seedless Procrustes#745

Merged
bdpedigo merged 2 commits into
devfrom
ldt-bug
Apr 1, 2021
Merged

Fix incorrect use of optimal transport params in Seedless Procrustes#745
bdpedigo merged 2 commits into
devfrom
ldt-bug

Conversation

@bdpedigo
Copy link
Copy Markdown
Collaborator

@bdpedigo bdpedigo commented Apr 1, 2021

  • Does this PR add any new dependencies?
  • Does this PR modify any existing APIs?
    • Is the change to the API backwards compatible?
  • Have you built the documentation (reference and/or tutorial) and verified the generated documentation is appropriate?

Reference Issues/PRs

None

What does this implement/fix? Briefly explain your changes.

Fixes a bug (I think?) where the wrong parameter was used in sinkhorn.
I haven't tested the effects of this much - but seems like they could be huge if eps is less than one. My guess is SeedlessProcrustes would always terminate after one iteration or something like that with the old code

Any other comments?

cc @tliu68 who found this issue
cc @alyakin314 who may have thoughts

@netlify
Copy link
Copy Markdown

netlify Bot commented Apr 1, 2021

Deploy preview for graspologic ready!

Built with commit aba299d

https://deploy-preview-745--graspologic.netlify.app

@bdpedigo bdpedigo changed the title Fix incorrect use of optimal transport params in LDT Fix incorrect use of optimal transport params in Seedless Procrustes Apr 1, 2021
Copy link
Copy Markdown
Contributor

@alyakin314 alyakin314 left a comment

Choose a reason for hiding this comment

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

looks fine.

my apologies for the slip.

@alyakin314
Copy link
Copy Markdown
Contributor

alyakin314 commented Apr 1, 2021

i was able to trace it to here:

https://github.com/PythonOT/POT/blob/f6139428e70ce964de3bef703ef13aa701a83620/ot/bregman.py#L388

(they import sinkhorn from ot.bergman in __init__, and that function is a wrapper for sinkhorn_knopp)

so my guess is that it indeed was only doing one iteration.

@alyakin314
Copy link
Copy Markdown
Contributor

yet simultaneously i find it somewhat hard to believe.

@bdpedigo
Copy link
Copy Markdown
Collaborator Author

bdpedigo commented Apr 1, 2021

yet simultaneously i find it somewhat hard to believe.

same, yet seems like this would matter for at least a few papers. and some stuff ive been doing now. also possible that 1 iteration is pretty good most of the time

@bdpedigo
Copy link
Copy Markdown
Collaborator Author

bdpedigo commented Apr 1, 2021

@j1c i wonder if this matters for improved nonpar paper, just a heads up

@alyakin314
Copy link
Copy Markdown
Contributor

yet simultaneously i find it somewhat hard to believe.

same, yet seems like this would matter for at least a few papers. and some stuff ive been doing now. also possible that 1 iteration is pretty good most of the time

paper-wise i can only think of Jaewon's. I haven't used it in mine, and Agterbreg was using his R implementation. I've played with a very few global iterations (it worked okay) but never with OT iterations.

@bdpedigo bdpedigo merged commit 25a9866 into dev Apr 1, 2021
@bdpedigo bdpedigo deleted the ldt-bug branch April 1, 2021 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants