-
Notifications
You must be signed in to change notification settings - Fork 129
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
[ENH] Mahalanobis distance #1351
base: main
Are you sure you want to change the base?
Conversation
Thank you for contributing to
|
Hi @krstopro thanks so much for the PR! Great to have a new distance! If you're struggling with anything in regards to the tests or CICD stuff feel free to shoot me a message on slack so we can get this in quicker. |
Hey @krstopro , This looks really good. Also from what I remember, including |
Oh, I see. I saw that the issue is open and decided to implement it. |
@krstopro Also, you rightly said that they've implemented the "squared" mahalanobis distance. In aeon, I think we should discuss it once if we want to implement the squared or a more general regular mahalanobis distance. |
I am not exactly sure on which Slack channel should I write. I've dropped you a message there. |
@chrisholder @aadya940 @krstopro Just checking in, everything good with this PR? Feel free to ask questions if you need some help. |
@MatthewMiddlehurst Yes, sorry. I was just busy and didn't write on Slack about it. Is it fine that I do it now? |
Yep, feel free to work at your own pace. I would ask in the #distances channel, but other channels you think are relevant are also fine. |
@krstopro @MatthewMiddlehurst Mahalanobis is primarily significant in outlier detection, we could mention it somewhere in the docstring once the PR is done? |
@chrisholder discussed fixing some of the testing issues on Slack. Just an FYI for others, not a push to do so asap 🙂. |
Just an update @krstopro and I spoke on slack and he is implementing an update to make the distance symmetric (it currently isnt) and then I will finalise the tests for the distance. |
Hi, any progress on the above @chrisholder @krstopro? |
@MatthewMiddlehurst Not yet, sorry. I hope to push by the end of the week. Apologies for the delay. |
no rush if you are busy, just checking in 🙂 |
This has got a bit stale, merged main into it. |
There have been a few distance updates recently, I think this would be outdated now? @chrisholder |
@krstopro we can now support the distances not returning the same value with different x, y orders (i.e. y, x). This means we can get this in. If you're still around reach out to me on the slack and we can work through this! |
Reference Issues/PRs
Fixes #1225.
What does this implement/fix? Explain your changes.
Implements Mahalanobis distance.
Any other comments?
Following "Learning a Mahalanobis Distance based Dynamic Time Warping Measure for Multivariate Time Series Classification" paper the square root is removed. Therefore, the distance implemented is actually the Mahalanobis distance squared. I am not sure about this.