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

Distance metric refinement #1664

Merged
merged 9 commits into from
Nov 30, 2021
Merged

Distance metric refinement #1664

merged 9 commits into from
Nov 30, 2021

Conversation

chrisholder
Copy link
Collaborator

@chrisholder chrisholder commented Nov 29, 2021

Reference Issues/PRs

What does this implement/fix? Explain your changes.

TLDR:
Improves the speed of the metrics and fixes a bug that occurred when using the numba fastmath flag and bounding matrix parameters. In addition the lower_bounding parameter has been removed and instead the lower_bounding technique to use is inferred based on the other parameters passed.

Full version:
Previously the distances weren't being cache'd as efficiently as possible and so the cache flag has been added to the factory methods to allow them to be cache'd for repeat calls.

In addition there was a bug caused by using the fastmath numba flag where values were being inserted into the wrong indexes of the bounding matrix (still not sure why this is caused). As such for now the fastmath flag has been removed.

Finally the lower_bounding parameter felt redundent as it could simply be inferred by which parameters are being passed and therefore the parameter was removed.

Does your contribution introduce a new dependency? If yes, which one?

What should a reviewer concentrate their feedback on?

Any other comments?

PR checklist

For all contributions
  • I've added myself to the list of contributors.
  • Optionally, I've updated sktime's CODEOWNERS to receive notifications about future changes to these files.
  • I've added unit tests and made sure they pass locally.
For new estimators
  • I've added the estimator to the online documentation.
  • I've updated the existing example notebooks or provided a new one to showcase how my estimator works.

Copy link
Collaborator

@fkiraly fkiraly left a comment

Choose a reason for hiding this comment

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

I think this is ok if we can get it in for the next release.
If the release happens before the merge, I would have major reservations because this would result in interface changes, so can we get this done quickly?

Besides that, I only have one minor comment: please remove any commented out code.

@TonyBagnall
Copy link
Contributor

I think this is ok if we can get it in for the next release. If the release happens before the merge, I would have major reservations because this would result in interface changes, so can we get this done quickly?

Besides that, I only have one minor comment: please remove any commented out code.

hi franz, what interface changes are there? All of thes numba distances are new, and all the existing code is still there

TonyBagnall
TonyBagnall previously approved these changes Nov 29, 2021
Copy link
Contributor

@TonyBagnall TonyBagnall left a comment

Choose a reason for hiding this comment

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

I have tested this for correctness and speed on dtw. Any further problems can be resolved by latter PRs, but I am confident this is now all correct.

@TonyBagnall TonyBagnall merged commit 25ba2f4 into main Nov 30, 2021
@TonyBagnall TonyBagnall deleted the distance-refinement branch November 30, 2021 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants