Skip to content
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

Merge all OT mapping methods in one class #114

Open
rflamary opened this issue Feb 28, 2024 · 2 comments
Open

Merge all OT mapping methods in one class #114

rflamary opened this issue Feb 28, 2024 · 2 comments
Labels
algorithm Implementation or improvements of the DA methods enhancement New feature or request

Comments

@rflamary
Copy link
Collaborator

Should we merge OT mapping methods?

it seems like the use of entropic regularization or group lasso could be added easily as a parameter to the same class. We should probably keep LinearOTMapping separate because it is very different in term of mapping but the OTMapping, EntropicOTmapping and Grouplasso could be merged in one class in my opinion and it would make SKADA less OT biased ;).

Another question is if we should move them in _ot.py . on this one I'm a little more circumspect since those are mapping method and they probably belong with CORAL...

@kachayev kachayev added enhancement New feature or request algorithm Implementation or improvements of the DA methods labels Feb 28, 2024
@kachayev
Copy link
Collaborator

I agree it would be more convenient to have all methods that rely on POT in a separate module. In such a way, we can make POT into an optional dependency.

Regarding merging OT mappings, are there separate papers that introduced them? I guess if we stick to 'one paper = one method' razor, we should keep wrappers. If that's not the case, we can surely squash all of them together.

@rflamary
Copy link
Collaborator Author

About papers : some papers propose very different models (the target shift and other implemented bu @antoinecollas for instance, or classifier/regerssor for JDOT) so one method per paper is probably not a good idea as a general rule.

But for OT methods, they have all been more or less introduced as variants in the OTML PAMI paper so it makes a lot of sens to factorize.

About POT as optional dependency this is a question because it would amputate a number of methods with a default install. I'm of course totally biased ;) . We have also several OT methds not based o mapping to implement so having both OT mapping and non maping together might not be best. maybe we can make _mapping as a folder and have diffeent files to separate OT and others?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
algorithm Implementation or improvements of the DA methods enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants