Use scipy cdist to calculate distance matrix #133
Conversation
|
No API break detected ✅ |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #133 +/- ##
==========================================
- Coverage 98.18% 98.17% -0.02%
==========================================
Files 13 13
Lines 661 656 -5
==========================================
- Hits 649 644 -5
Misses 12 12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
IAlibay
left a comment
IAlibay
left a comment
There was a problem hiding this comment.
Couple things, mostly re: intent on adding extra arguments if we aren't planning to ever use them internally.
Also inconsistencies in typing.
| ] = vector_eucledean_dist, | ||
| ] | ||
| | str = "euclidean", | ||
| metric_kwargs: dict = {}, |
There was a problem hiding this comment.
Why is this being added? It's a private method, so unless you're planning on expanding the method it's best to assume it will never be used.
| position matrix B | ||
| metric : Callable[[Union[float, Iterable], Union[float, Iterable]], | ||
| Union[float, Iterable]], optional | ||
| Union[float, Iterable]] | str, optional |
There was a problem hiding this comment.
This doesn't match your signature. Again, unless we intend someone to pass through a str here (i.e. it will be called somewhere lese in the class for other reasons), then I would just leave it as a callable?
Also, it's not optional by function kwarg typing, should it be optional here?
| metric: eucledean distance. See | ||
| https://docs.scipy.org/doc/scipy/reference/generated/scipy.spatial.distance.cdist.html#scipy.spatial.distance.cdist | ||
| for other options. | ||
| metric_kwargs: dict, optional |
There was a problem hiding this comment.
As above, also the function typing isn't opptional, should it be?
| https://docs.scipy.org/doc/scipy/reference/generated/scipy.spatial.distance.cdist.html#scipy.spatial.distance.cdist | ||
| for other options. | ||
| metric_kwargs: dict, optional | ||
| Extra arguemnts to metric, see https://docs.scipy.org/doc/scipy/reference/generated/scipy.spatial.distance.cdist.html#scipy.spatial.distance.cdist |
There was a problem hiding this comment.
| Extra arguemnts to metric, see https://docs.scipy.org/doc/scipy/reference/generated/scipy.spatial.distance.cdist.html#scipy.spatial.distance.cdist | |
| Extra arguements to metric, see https://docs.scipy.org/doc/scipy/reference/generated/scipy.spatial.distance.cdist.html#scipy.spatial.distance.cdist |
|
I was trying to match the functionality of the old way it worked (you could pass in a callable) and merge it with cdist (you can pass in a string OR a callable). I don't think we really need to support different metrics, so I rather just hard-code Euclidean and make this much more simple, is that okay @IAlibay ? |
Works for me! Fewer options == less testing needed! |
2 participants
Checklist
newsentry, or the changes are not user-facing.pre-commit.ci autofix.Developers certificate of origin
Fixes issue #57