Repository navigation
Clarify Mahalanobis documentation - #272
davide-grheco wants to merge 2 commits into
Conversation
The Mahalanobis function passes the provided matrix to the Mahalanobis formula without inverting it. A user would generally expect the function to accept the covariance matrix and that is what the documentation said. However, to obtain the Mahalanobis distance it is required to pass the inverse of the covariance matrix as the function is currently formulated.
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## master #272 +/- ##
===========================================
- Coverage 95.86% 79.04% -16.83%
===========================================
Files 10 10
Lines 944 606 -338
===========================================
- Hits 905 479 -426
- Misses 39 127 +88 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| Create a Mahalanobis distance (i.e., a bilinear form) with covariance matrix `Q`. | ||
| Create a Mahalanobis distance `sqrt((x - y)' * Q * (x - y))` | ||
| with positive semidefinite matrix `Q`. Note that `Q` enters the bilinear form directly: | ||
| to obtain the classical Mahalanobis distance, pass the inverse of the covariance |
There was a problem hiding this comment.
Is it really correct to oppose "the classical Mahalanobis distance" to this method's definition? AFAICT this is also the classical distance, it's just that the function expects a different input from the standard. How about "to obtain the Mahalanobis distance from the covariance matrix S, pass its inverse, i.e. Mahalanobis(inv(S))."?
|
|
||
| Create a Mahalanobis distance (i.e., a bilinear form) with covariance matrix `Q`. | ||
| Create a Mahalanobis distance `sqrt((x - y)' * Q * (x - y))` | ||
| with positive semidefinite matrix `Q`. Note that `Q` enters the bilinear form directly: |
There was a problem hiding this comment.
"enters" seems weird here: maybe "specifies"?
Also probably worth adding an empty line so that the short definition is in a separate paragraph.
| @@ -240,7 +240,9 @@ arrays of real numbers; `w` is an iterator/array of parameters (like weights or | |||
| periods); `k` and `l` are iterators/arrays of distinct elements of | |||
| any kind; `a` and `b` are iterators/arrays of Bools; and finally, `p` and `q` are | |||
There was a problem hiding this comment.
"finally" isn't appropriate anymore. Also below better continue the same sentence with a semicolon for consistency.
| enters the bilinear form directly: to obtain the classical Mahalanobis distance, | ||
| pass the inverse of the covariance matrix `S`, i.e. `mahalanobis(x, y, inv(S))`. |
There was a problem hiding this comment.
This seems to specific to mention here (we can't detail all methods), people can look at the docstring if needed.
| enters the bilinear form directly: to obtain the classical Mahalanobis distance, | |
| pass the inverse of the covariance matrix `S`, i.e. `mahalanobis(x, y, inv(S))`. | |
| specifies the bilinear form (inverse of covariance matrix). |
Made the readme explanation of the mahalanobis distance parameter simpler. Clarify that `Q` is the precision matrix, which is the inverse of the covariance matrix `S`.
|
Sorry for the delay. I modified the documentation, now it clarifies that |
2 participants
The Mahalanobis function passes the provided matrix to the Mahalanobis formula without inverting it. A user would generally expect the function to accept the covariance matrix and that is what the documentation said. However, to obtain the Mahalanobis distance it is required to pass the inverse of the covariance matrix as the function is currently formulated.
This closes issue #244
In my opinion it would be better if the Mahalanobis function did the inversion of the matrix. The user would likely expect the Mahalanobis function to accept the covariance matrix rather than requiring the inverse. However I do understand this may break plenty downstream uses of the library.