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

Add MinkowskiMetric abstract type #265

Merged
merged 9 commits into from
Oct 23, 2024
Merged

Conversation

eliascarv
Copy link
Contributor

This new type will allow third-party packages to differentiate Minkowski metrics at the type level. It will also allow third-party packages to implement their own Minkowski metrics.

A practical example of a package that could use this new abstract type is NearestNeighbors.jl, which is currently using a union type for this purpose:

https://github.com/KristofferC/NearestNeighbors.jl/blob/master/src/kd_tree.jl#L1

With this new type NearestNeighbors.j could use Minkowski metrics defined in any package.

@juliohm
Copy link
Contributor

juliohm commented Oct 15, 2024

Appreciate if a maintainer can review and merge this.

@juliohm
Copy link
Contributor

juliohm commented Oct 17, 2024

ping @KristofferC @dkarrasch

Copy link
Member

@dkarrasch dkarrasch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem to hurt anyone, so this LGTM, except for the consideration of adding two types vertically into the type hierarchy.

src/metrics.jl Show resolved Hide resolved
@eliascarv eliascarv requested a review from dkarrasch October 17, 2024 12:25
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.50%. Comparing base (886ad02) to head (c9587a1).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #265      +/-   ##
==========================================
+ Coverage   95.86%   96.50%   +0.64%     
==========================================
  Files          10       10              
  Lines         944      888      -56     
==========================================
- Hits          905      857      -48     
+ Misses         39       31       -8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@dkarrasch dkarrasch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks much cleaner now and non-breaking.

README.md Outdated Show resolved Hide resolved
eliascarv and others added 2 commits October 17, 2024 16:06
@eliascarv eliascarv requested a review from dkarrasch October 17, 2024 19:09
Copy link
Member

@dkarrasch dkarrasch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dkarrasch dkarrasch requested a review from nalimilan October 17, 2024 19:19
@eliascarv
Copy link
Contributor Author

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

src/metrics.jl Outdated Show resolved Hide resolved
@eliascarv eliascarv requested a review from nalimilan October 21, 2024 16:49
src/generic.jl Outdated Show resolved Hide resolved
Co-authored-by: Milan Bouchet-Valat <[email protected]>
src/Distances.jl Outdated Show resolved Hide resolved
@dkarrasch dkarrasch merged commit 35c6d0d into JuliaStats:master Oct 23, 2024
9 checks passed
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.

5 participants