-
Notifications
You must be signed in to change notification settings - Fork 15
Implemented Various Distance Metric #107
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
base: main
Are you sure you want to change the base?
Conversation
This reverts commit 16031e4.
uttammittal02
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Add docs for distance metrics
- Resolve the conflicts
- Shift all the files from neighbours to metrics
src/slowmokit/methods/neighbors/distance_metric/distance_metric.cpp
Outdated
Show resolved
Hide resolved
src/slowmokit/methods/neighbors/distance_metric/distance_metric.cpp
Outdated
Show resolved
Hide resolved
src/slowmokit/methods/neighbors/distance_metric/distance_metric.cpp
Outdated
Show resolved
Hide resolved
src/slowmokit/methods/neighbors/distance_metric/distance_metric.cpp
Outdated
Show resolved
Hide resolved
src/slowmokit/methods/neighbors/distance_metric/distance_metric.cpp
Outdated
Show resolved
Hide resolved
src/slowmokit/methods/neighbors/distance_metric/distance_metric.cpp
Outdated
Show resolved
Hide resolved
src/slowmokit/methods/neighbors/distance_metric/distance_metric.cpp
Outdated
Show resolved
Hide resolved
src/slowmokit/methods/neighbors/distance_metric/distance_metric.hpp
Outdated
Show resolved
Hide resolved
uttammittal02
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few more changes
src/slowmokit/methods/metrics/distance_metric/distance_metric.cpp
Outdated
Show resolved
Hide resolved
src/slowmokit/methods/metrics/distance_metric/distance_metric.cpp
Outdated
Show resolved
Hide resolved
uttammittal02
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add the docs for distance metrics bruh
uttammittal02
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last change ig
| int n = x.size(); | ||
| for (int i = 0; i < n; i++) | ||
| { | ||
| distance += std::pow(x[i] - y[i], power); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use std::abs(x[i] - y[i]) instead.
Otherwise there might be a problem in case of odd values of p.
uttammittal02
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm
| ## Manhattan Distance | ||
| Manhattan Distance is the sum of absolute differences between points across all the dimensions. | ||
|
|
||
| ## Manhattan Distance |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change distance name to minkowski
| Minkowski Distance is the generalized form of Euclidean and Manhattan Distance. | ||
|
|
||
| ## Cosine Similarity | ||
| Cosine similarity is a metric, helpful in determining, how similar the data objects are irrespective of their size. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add sentence like
lesser is the cosine similarity, more similar are two points.
for all four metric.
| #include "distance_metric.hpp" | ||
|
|
||
| template<class T> | ||
| DistanceMetric<T>::DistanceMetric(std::vector<T> &x, std::vector<T> &y) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| DistanceMetric<T>::DistanceMetric(std::vector<T> &x, std::vector<T> &y) | |
| DistanceMetric<T>::DistanceMetric(const std::vector<T> &x, const std::vector<T> &y) |
We don't want user to feel insecure about their data. Use const. (No need to update in .hpp file)
const in cpp but not in hpp?
.hpp file
void f(int);.cpp file
void f(const int param1) {
return;
}is valid and recommended.
| DistanceMetric<T>::DistanceMetric(std::vector<T> &x, std::vector<T> &y) | ||
| { | ||
| this->x = x; | ||
| this->y = y; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| DistanceMetric<T>::DistanceMetric(std::vector<T> &x, std::vector<T> &y) | |
| { | |
| this->x = x; | |
| this->y = y; | |
| DistanceMetric<T>::DistanceMetric(std::vector<T> &x, std::vector<T> &y) : x(x), y(y) | |
| { |
Use initialiser list, they are faster.
| * @param x | ||
| * @param y |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add short statement describing x and y
|
|
||
| /** | ||
| * @param power The order of the norm | ||
| * @returns minkowski distance between the two vectors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * @returns minkowski distance between the two vectors | |
| * @returns minkowski distance between the two vectors | |
| * @throws ... |
power < 1 is not allowed..
|
|
||
| template<class T> double DistanceMetric<T>::magnitude(std::vector<T> &x) | ||
| { | ||
| double result = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| double result = 0; | |
| long double result = 0; |
double might overflow
|
|
||
| template<class T> double DistanceMetric<T>::cosineSimilarity() | ||
| { | ||
| double dotProduct = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| double dotProduct = 0; | |
| long double dotProduct = 0; |
|
|
||
| template<class T> double DistanceMetric<T>::euclidean() | ||
| { | ||
| double distance = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| double distance = 0; | |
| long double distance = 0; |
You will have to downcast long double to double finally. use std::dynamic_cast
UPDATE: Do this wherever I have added double -> long double comments
src/slowmokit/methods/metrics/distance_metric/distance_metric.hpp
Outdated
Show resolved
Hide resolved
|
Fix conflicts as well. @Modernbeast02 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commit names should be relevant from next time
ken1000minus7
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say it would be better to make all functions static instead of passing the vectors as property and then calling respective functions
harshjohar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revert CMakeLists.txt and don't change it in future models
Resolved Issue #95