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

minkowski distance #308

Merged
merged 2 commits into from
Oct 6, 2023
Merged

minkowski distance #308

merged 2 commits into from
Oct 6, 2023

Conversation

s-weil
Copy link
Contributor

@s-weil s-weil commented Oct 3, 2023

Please reference the issue(s) this PR is related to

Closes #182

Please list the changes introduced in this PR

  • Minkowski distance in DistanceMetrics.fs for vectors, arrays and sequences

Description

Implement the Minkowski distance of order p in several versions, following https://en.wikipedia.org/wiki/Minkowski_distance and #182

[Required] please make sure you checked that

  • [ x] The project builds without problems on your machine

[Optional]

  • [ x] Added unit tests regarding the added features

implement the minkowski distance of order p in several versions. add unit tests
@s-weil s-weil mentioned this pull request Oct 3, 2023
@s-weil
Copy link
Contributor Author

s-weil commented Oct 3, 2023

Note that in a separate issue, one could try to tackle code duplications in the src and tests of distance metrics.
This however may affect performance and must be conducted carefully.

let mutable acc = 0.0

for i in 0 .. (dim - 1) do
let abs =
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason you didnt use abs (v1.[i] - v2.[i]) to determine the absolute difference?

Copy link
Contributor Author

@s-weil s-weil Oct 4, 2023

Choose a reason for hiding this comment

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

Other functions, such as cityblockNaN used System.Math.Abs
So I wanted to keep it aligned, but I am more than happy to change it

Updated

Copy link
Member

@bvenn bvenn left a comment

Choose a reason for hiding this comment

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

Thanks for the addition! From the Wikipedia diagram I assume p could also be 0 < p <= 1, but I did not went deeper into research. Is there a reason you constrain p to be an integer? Or did I miss something?

src/FSharp.Stats/DistanceMetrics.fs Outdated Show resolved Hide resolved
@s-weil
Copy link
Contributor Author

s-weil commented Oct 4, 2023

Regarding that p could also be 0 < p < 1 and restricting it to be an integer, I may have been a bit too strict on the given signature specified in #296
Also it was specified to be an integer in https://en.wikipedia.org/wiki/Minkowski_distance

Happy to change it to a float for a more generic version. It may affect performance though.

If it becomes a float, I can also handle the case when 0 < p <1 by removing the exponent 1/p as described in Wiki.

* replace System.Math functions
* correct summaries
* gernalize order p to be a float > 0
* extended unit tests
@s-weil
Copy link
Contributor Author

s-weil commented Oct 5, 2023

Pushed the feedback.

The order p is now a float and the case 0 < p < 1 is covered

@codecov-commenter
Copy link

Codecov Report

Merging #308 (9d38186) into developer (1e680b1) will increase coverage by 0.27%.
Report is 12 commits behind head on developer.
The diff coverage is 79.57%.

@@              Coverage Diff              @@
##           developer     #308      +/-   ##
=============================================
+ Coverage      46.73%   47.00%   +0.27%     
=============================================
  Files            148      148              
  Lines          16225    16458     +233     
  Branches        2196     2219      +23     
=============================================
+ Hits            7582     7736     +154     
- Misses          7987     8052      +65     
- Partials         656      670      +14     
Files Coverage Δ
tests/FSharp.Stats.Tests/DistanceMetrics.fs 100.00% <100.00%> (ø)
src/FSharp.Stats/DistanceMetrics.fs 25.23% <43.52%> (-15.68%) ⬇️

... and 71 files with indirect coverage changes

@bvenn bvenn merged commit 4719e96 into fslaborg:developer Oct 6, 2023
2 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.

Minkowski distance
3 participants