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

Stricter check_params protocol for DA module would simplify debugging #528

Open
kachayev opened this issue Oct 1, 2023 · 1 comment
Open

Comments

@kachayev
Copy link
Collaborator

kachayev commented Oct 1, 2023

Describe the bug

ot.utils.check_params is used rather heavily throughout ot.da functions. The function itself checks for None(s) in the input and returns False if at least one was spotted. Now, there are 2 patters of how check_params is used in the code that are somewhat hard to debug when argument passed is, in fact, "invalid":

  1. fit function works when check_params returns True, otherwise just returns self (example). As there's no exceptions thrown, the downstream code safely assumes that fitting is done.
  2. transform returns the result of the transformation only in case check_params returns True, otherwise returns None (example).

In both cases, it's rather hard to find the cause as the issue is only reported using print statements.

Expected behavior

Suggestion here is rather simple: to follow stricter convention, for example similar to check_array in sklearn. Throwing ValueError in case invalid argument is provided seems to be safer option. What do you think? Happy to make a PR with the changes.

@rflamary
Copy link
Collaborator

Make sens, we should also probably move check_param in ot.da and change its name that is too generic.

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

No branches or pull requests

2 participants