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

Use the new MinkowskiMetric type from Distances.jl #206

Merged
merged 5 commits into from
Nov 12, 2024

Conversation

eliascarv
Copy link
Contributor

No description provided.

src/NearestNeighbors.jl Outdated Show resolved Hide resolved
Project.toml Show resolved Hide resolved
src/NearestNeighbors.jl Outdated Show resolved Hide resolved
src/NearestNeighbors.jl Outdated Show resolved Hide resolved
@eliascarv eliascarv requested a review from dkarrasch October 24, 2024 17:05
@dkarrasch
Copy link
Contributor

In your local copy, you'll need to switch the environment to benchmark and then benchmark> up (in pkg mode) update the manifest (otherwise it will not catch the latest Distances.jl) and then push the changes to Manifest.toml to this branch.

@eliascarv
Copy link
Contributor Author

Done!

@eliascarv
Copy link
Contributor Author

eliascarv commented Oct 25, 2024

@dkarrasch, I just noticed something while reading the KDTree code: the implementation uses functions from the UnionMetric API, which are: eval_reduce and eval_end. In other words, it will not work with any MinkowskiMetric.
That is, only UnionMinkowskiMetric are supported in the KDTree code.

image

src/NearestNeighbors.jl Outdated Show resolved Hide resolved
@eliascarv
Copy link
Contributor Author

@dkarrasch, what do you think about moving the eval_pow and eval_diff functions to Distances.jl?
These functions seem to be part of the UnionMetrics API.
Or do you think these functions are too specific and are only for NearestNeighbors.jl internal use?

@eliascarv
Copy link
Contributor Author

@KristofferC, can you review this PR please? It has already been approved by Daniel.

@dkarrasch
Copy link
Contributor

@dkarrasch, what do you think about moving the eval_pow and eval_diff functions to Distances.jl? These functions seem to be part of the UnionMetrics API. Or do you think these functions are too specific and are only for NearestNeighbors.jl internal use?

I'm not sure. Aren't they only used for UnionMinkowskiMetrics? The other thing is that there is no documented UnionMetrics API, right? You need to distill it from the Distances.jl code.

I think one would need concrete reasons to change something that has been up and running for so long. The risk of breakage is too big, I'd say.

@juliohm
Copy link
Contributor

juliohm commented Oct 31, 2024

ping @KristofferC

@eliascarv
Copy link
Contributor Author

@KristofferC, this PR is ready. Can you review it?

@juliohm
Copy link
Contributor

juliohm commented Nov 11, 2024

Is there anything we can do to get this merged? @KristofferC could you give @dkarrasch permissions to maintain this package?

@KristofferC
Copy link
Owner

All I can do is blame the horrible github notification UI for not showing me this earlier.

@KristofferC KristofferC merged commit 8867613 into KristofferC:master Nov 12, 2024
8 of 9 checks passed
@eliascarv eliascarv deleted the minkowski branch November 12, 2024 11:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants