-
Notifications
You must be signed in to change notification settings - Fork 529
[MRG] Adds Unbalaced transport to domain adaptation methods + bugfixes #100
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…ll bug related to warnings in unbalaced.py . Added an error message when user wants to normalize with other than expected cost normalization functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. The reg_e
and reg_m
are perhaps more adequate (than reg and alpha) with the naming convention of pot.
Can you please duplicate this test for your UnbalancedSinkhorn class ?
https://github.com/rflamary/POT/blob/b2157e9b3458388571f6ae87d80f47f500dfa166/test/test_da.py#L169
|
||
Parameters | ||
---------- | ||
reg_e : float, optional (default=1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps the default reg_e
should be smaller ? or None and set to 10 / dimension for e.g in fit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, but since by default norm = None, maybe we could leave it like that to avoid numerical errors in the default run?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I just realized that I have to check if the norm is None and do no normalization in that case, otherwise the error message will be triggered...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok it is done and it seems ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I just realized the other da
classes use the same default
ot/da.py
Outdated
""" | ||
|
||
def __init__(self, reg_e=1., reg_m=0.1, method='sinkhorn', | ||
max_iter=10, tol=10e-9, verbose=False, log=False, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this tol
is 1e-8 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh yes. Actually the default value is 1e-9. Will fix.
Shoot, there are still some syntax error and my flake8 and pydocstyle are buggy, I have to find a way to fix them |
Aaah finally. We are green. |
Thank you @ngayraud, This PR looks very nice. I will have a quick look in the next days and will do the merge. |
This PR is primarily adds the unbalanced sinkhorn method to the domain adaptation classes. When the other sinkhorn methods will be implemented (eg stabilized) they will also be supported by the parameter method, which will now raise an error according to what is in unbalanced.py
In addition: