-
Notifications
You must be signed in to change notification settings - Fork 25
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
Streamlining Distance API #416
Comments
Seems reasonable. No immediate problems this could cause come to my mind. |
Two issues come to mind:
but with the proposed change, I would call
In the proposed implementation, you would
In my opinion, this is considerably less readable and not intuitive. It also doesn't just apply to
And just to clarify, you're thinking of using it like
NOT
Right? |
Discussed a few things with @yugeji
|
For future distances that do not use what is currently the standard |
An important addition to this refactor (which would also allow |
Description of feature
This is a continuation of #405 but specific for
Distance
.TLDR: Currently,
Distance
does not adhere to the API design of the rest of pertpy and I want to harmonize it. Currently, we pass ametric
to the constructor which then uses the appropriate distance function on__call__
. This comes with two issues:Currently we also have the
onesided_distances
pairwise
precompute_distances
functions.
Moving
metric
into these 3 functions wouldn't really help or solve any issue. The only option I see is having functions like:distance.compute_wasserstein(mode=Literal'onesided', 'pairwise', 'precompute'])
for all of the metrics. These would then show up in a table of functions and can be documented more easily. It would also probably correspond better with the current design.What do you think? I'm especially interested in @yugeji, @stefanpeidli, and @tessadgreen opinion.
The text was updated successfully, but these errors were encountered: