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

[ENH] Minkowski distance #904

Merged
merged 20 commits into from
Dec 11, 2023
Merged

Conversation

akshatvishu
Copy link
Contributor

@akshatvishu akshatvishu commented Nov 14, 2023

Reference Issues/PRs

Fixes #484

What does this implement/fix? Explain your changes.

This PR introduces a new distance measure minkowski_distance.

Any other comments?

PR checklist

For all contributions
  • I've added myself to the list of contributors. Alternatively, you can use the @all-contributors bot to do this for you.
  • The PR title starts with either [ENH], [MNT], [DOC], [BUG], [REF] or [GOV] indicating whether the PR topic is related to enhancement, maintenance, documentation, bugs, refactoring or governance.
For new estimators and functions
For developers with write access
  • Optionally, I've updated aeon's CODEOWNERS to receive notifications about future changes to these files.

@aeon-actions-bot aeon-actions-bot bot added distances Distances package enhancement New feature, improvement request or other non-bug code enhancement labels Nov 14, 2023
@aeon-actions-bot
Copy link
Contributor

Thank you for contributing to aeon!

I have added the following labels to this PR based on the title: [ $\color{#FEF1BE}{\textsf{enhancement}}$ ].
I have added the following labels to this PR based on the changes made: [ $\color{#5209C9}{\textsf{distances}}$ ]. Feel free to change these if they do not properly represent the PR.

The Checks tab will show the status of our automated tests. You can click on individual test runs in the tab or "Details" in the panel below to see more information if there is a failure.

If our pre-commit code quality check fails, any trivial fixes will automatically be pushed to your PR unless it is a draft.

Don't hesitate to ask questions on the aeon Slack channel if you have any!

@MatthewMiddlehurst
Copy link
Member

Hello, sorry for the late reply. The people who are more familiar with distances are a bit busy at the moment.

There appears to be an issue with your example, the output does not patch the expected https://github.com/aeon-toolkit/aeon/actions/runs/6869352045/job/18682151763?pr=904#step:6:399

@akshatvishu
Copy link
Contributor Author

Hello, sorry for the late reply. The people who are more familiar with distances are a bit busy at the moment.

There appears to be an issue with your example, the output does not patch the expected https://github.com/aeon-toolkit/aeon/actions/runs/6869352045/job/18682151763?pr=904#step:6:399

Thanks for pointing it out! I will immediately correct it.

@TonyBagnall
Copy link
Contributor

him, thanks for this, we will look early next week @chrisholder

@MatthewMiddlehurst
Copy link
Member

Hi @akshatvishu sorry about the delay, people seem to still be busy. I will take a look at this later today.

@chrisholder
Copy link
Contributor

Hi @akshatvishu thanks for the contribution! Look great just a few more things to add so people can call your distance properly:

  1. Add your distance to the valid distances array located at the bottom of aeon/distances/_distance.py. Will look something like:
    {
        "name": "minkowski",
        "distance": minkowski_distance,
        "pairwise_distance": minkowski_pairwise_distance,
    },
  1. Add your distance to the utility functions in aeon/distances/_distance.py. Add to the following functions:
  • line 87 distance function
  • line 227 pairwise_distance function
  1. Add your distance to the various docstring tables in aeon/distances/_distance.py. You will need to add it to the following functions:
  • Line 705 get_distance_function
  • Line 757 get_pairwise_distance_function

Copy link
Member

@MatthewMiddlehurst MatthewMiddlehurst left a comment

Choose a reason for hiding this comment

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

Hi, the distance itself looks good. I see Chris left some comments for incorporating it into the module, which would be good to add.

I only have a few documentation comments.

It would be good to add the functions to docs/api_reference/distance.rst (this one is more optional and can be dealt with in a separate PR).

- Integrated Minkowski distance into valid distances array in _distance.py.
- Enhanced distance and pairwise_distance functions in _distance.py for Minkowski distance.
- Updated docstring tables in _distance.py to reflect Minkowski distance addition.
- Corrected docstring example for minkowski_pairwise_distance  at _minkowski.py to correct format.
- Removed the use of  #noqa: E501 to skip code quality check at _minkowski_pairwise_distance function.
@akshatvishu
Copy link
Contributor Author

Hi @chrisholder and @MatthewMiddlehurst,
Thank you both for your valuable feedback during the initial review. I've carefully considered and incorporated all the suggested changes into the latest commits. Hopefully everything will work fine this time!

Copy link
Member

@MatthewMiddlehurst MatthewMiddlehurst 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 changes. My bits seem done, adding the distances to the parts Chris suggested seems to have included it in a set of unit tests (which is now failing).

Do you know what the issue is @chrisholder? I can have a look at some point if not.

If you want, you can install the dev dependencies (i.e. pip install .[dev]) and run the test suite locally using pytest.

@MatthewMiddlehurst
Copy link
Member

Looks like the tests are passing now. Would you mind taking a look through @chrisholder just to make sure everything is still good before merging?

chrisholder
chrisholder previously approved these changes Dec 7, 2023
Copy link
Contributor

@chrisholder chrisholder left a comment

Choose a reason for hiding this comment

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

Great addition to the distance module thanks for the hard work!

@TonyBagnall
Copy link
Contributor

hi, there is a minor conflict I introduced in another PR, easy to fix, just add manhattan.squared_distance. Ready to go in once thats fixed, thanks @akshatvishu

@MatthewMiddlehurst
Copy link
Member

I will resolve the conflict and merge this for the release. Thanks for the contribution @akshatvishu (and dealing with testing woes)!

@TonyBagnall TonyBagnall merged commit 8656d69 into aeon-toolkit:main Dec 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
distances Distances package enhancement New feature, improvement request or other non-bug code enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ENH] implement Minkowski distance function
4 participants