Skip to content

Defaults only exist for Euclidean impls #174

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

Open
montanalow opened this issue Sep 26, 2022 · 1 comment
Open

Defaults only exist for Euclidean impls #174

montanalow opened this issue Sep 26, 2022 · 1 comment
Labels
good first issue Good for newcomers
Milestone

Comments

@montanalow
Copy link
Collaborator

The following implementations use a generic impl Distance parameter, but only provide a Default impl for the Euclidean distance.

  • KNNClassifier
  • KNNRegressor
  • DBSCAN
  • KMeans

In one way this is nice, because it makes Euclidean the default at the generic level, so that types may be omitted completely, and only inferred during construction. However, this is problematic when parsing partial defaults from JSON, since if the distance type may no longer be inferred, similar to the currently required type annotations required during deserialization where they also cannot be inferred.

a) One fix would be to make Default::defaults generic for the Distance, and require the annotation during construction as well. This means all Distances would get defaults, but doesn't help the JSON w/ default construction case much, since we'd still need to first parse the JSON to get the get the type to get the defaults, and then match.

b) Another possibility would be to approach similar to #169 by making Distances an enum Struct, rather than a generic type. With that avenue we could still have a Euclidean default without type annotations, but also allow for partial JSON construction for non Euclidean defaults. This comes at the cost of the enum Struct reserving the space for the two Matrix params in Mahalanobis distance in all cases.

My preference would be for b since I believe it's a nicer DevX at a small memory cost that shouldn't impact performance.

@montanalow
Copy link
Collaborator Author

I'll draft up B to see what it looks like, and how well it actually works, unless there are objections.

@Mec-iS Mec-iS added this to the v0.5 milestone Oct 19, 2022
@Mec-iS Mec-iS added the good first issue Good for newcomers label Nov 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants