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

Added MinkowskiDistance support #1362

Merged
merged 66 commits into from
Feb 24, 2023
Merged

Conversation

clueless-skywatcher
Copy link
Contributor

What does this PR do?

Fixes #1345

Adds the Minkowski Distance metric as a regression metric. Adds both functional and object-oriented versions of the metric.

Before submitting

  • Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure to update the docs?
  • Did you write any new necessary tests?

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Absolutely! As a first-time contributor I got to learn more things and feeling proud that I am beginning to be a part of a huge open-source community.

Copy link
Member

@SkafteNicki SkafteNicki left a comment

Choose a reason for hiding this comment

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

PR is already looking good :]

src/torchmetrics/functional/__init__.py Show resolved Hide resolved
src/torchmetrics/functional/regression/minkowski.py Outdated Show resolved Hide resolved
src/torchmetrics/functional/regression/minkowski.py Outdated Show resolved Hide resolved
src/torchmetrics/regression/minkowski.py Outdated Show resolved Hide resolved
src/torchmetrics/regression/minkowski.py Show resolved Hide resolved
src/torchmetrics/functional/regression/minkowski.py Outdated Show resolved Hide resolved
src/torchmetrics/regression/minkowski.py Outdated Show resolved Hide resolved
tests/unittests/regression/test_minkowski_distance.py Outdated Show resolved Hide resolved
tests/unittests/regression/test_minkowski_distance.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Nov 27, 2022

Codecov Report

Merging #1362 (fd9053d) into master (935628b) will decrease coverage by 48%.
The diff coverage is 67%.

Additional details and impacted files
@@           Coverage Diff            @@
##           master   #1362     +/-   ##
========================================
- Coverage      86%     39%    -48%     
========================================
  Files         216     219      +3     
  Lines       11359   11473    +114     
========================================
- Hits         9802    4449   -5353     
- Misses       1557    7024   +5467     

@stancld
Copy link
Contributor

stancld commented Nov 28, 2022

One question before a deeper review: As Minkowsi Distance is a generalization of Euclidean and Manhattan distance, shouldn't this metric for consistency be included rather in pairwise metrics as the other two?

@Borda
Copy link
Member

Borda commented Nov 30, 2022

As Minkowsi Distance is a generalization of Euclidean and Manhattan distance, shouldn't this metric for consistency be included rather in pairwise metrics as the other two?

good question, I would move it to pairwise 🐰

@clueless-skywatcher
Copy link
Contributor Author

In that case, should I add the pairwise metric while keeping the functional and class metrics?

@stancld
Copy link
Contributor

stancld commented Dec 4, 2022

@Borda @clueless-skywatcher Actually, I don't know why we don't have module metrics for pairwise metrics. @Borda, is there any reason for this? If not, would it make sense to add class metrics in a separate PR?

@clueless-skywatcher
Copy link
Contributor Author

Yes @stancld I also think I should add a pairwise metric. The only hold-up is whether to keep the class and functional metrics alongwith the pairwise distance or not, and as @stancld mentioned, in the same PR or not.

@Borda
Copy link
Member

Borda commented Dec 7, 2022

@stancld @clueless-skywatcher I think it is a bit tricky as most of the regression metrics could be seen also as pairwise metrics, right?
But I do not have anything against establishing this domain in TM 🐰
cc: @Lightning-AI/core-metrics

@SkafteNicki
Copy link
Member

@stancld @Borda the core reason that I did not implement the pairwise metrics as modular long time ago, is that the size of the output differ depending on the size of the input:

X = torch.randn(N, d)
Y = torch.randn(M, d)
out = pairwise(X, y) # shape [N,M]

if N, M can differ from batch to batch it is difficult to accumulate.

That said, we could settle on the modular version always doing some kind of reduction:

X = torch.randn(N, d)
Y = torch.randn(M, d)
out = Pairwise(reduction='sum')(X, y) # shape [1,]

What do you think?

@Borda
Copy link
Member

Borda commented Dec 23, 2022

That said, we could settle on the modular version always doing some kind of reduction

I am fine with both options 🦦
cc: @justusschock @stancld @Lightning-AI/core-metrics

@mergify mergify bot added the ready label Feb 24, 2023
@Borda Borda merged commit b5aaa60 into Lightning-AI:master Feb 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Minkowski distance
6 participants