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

[BUG] in ClaSPSegmentation, deal with k when it is too large for np.argpartition #5490

Merged
merged 2 commits into from Oct 27, 2023

Conversation

Alex-JG3
Copy link
Contributor

Reference Issues/PRs

Closes #5476.

What does this implement/fix? Explain your changes.

Sometimes the ClaSPSegmentation algorithm raises a value error in the _compute_distances_iterative method. This happens when _compute_distances_iterative attempts to use np.argpartition with a kth value which is larger than the length of the input array.

This PR checks if k is larger than the length of dist and if it is, it sets k to be the length of dist minus 1.

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

No.

What should a reviewer concentrate their feedback on?

Did you add any tests for the change?

Any other comments?

PR checklist

For all contributions
  • I've added myself to the list of contributors with any new badges I've earned :-)
    How to: add yourself to the all-contributors file in the sktime root directory (not the CONTRIBUTORS.md). Common badges: code - fixing a bug, or adding code logic. doc - writing or improving documentation or docstrings. bug - reporting or diagnosing a bug (get this plus code if you also fixed the bug in the PR).maintenance - CI, test framework, release.
    See here for full badge reference
  • Optionally, I've added myself and possibly others to the CODEOWNERS file - do this if you want to become the owner or maintainer of an estimator you added.
    See here for further details on the algorithm maintainer role.
  • The PR title starts with either [ENH], [MNT], [DOC], or [BUG]. [BUG] - bugfix, [MNT] - CI, test framework, [ENH] - adding or improving code, [DOC] - writing or improving documentation or docstrings.

fkiraly
fkiraly previously approved these changes Oct 25, 2023
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.

Makes sense.

The way I would solve it is _k = min(k, len(dist)-1) and then use _k rather than if/else, but either is fine ofc.

@fkiraly fkiraly added module:annotation enhancement Adding new functionality labels Oct 25, 2023
@fkiraly fkiraly changed the title [BUG] Deal with 'k' when it is too large for 'np.argpartition' [BUG] in ClaSPSegmentation, deal with k when it is too large for np.argpartition Oct 25, 2023
@fkiraly fkiraly added bugfix Fixes a known bug or removes unintended behavior and removed enhancement Adding new functionality labels Oct 25, 2023
@Alex-JG3
Copy link
Contributor Author

The way I would solve it is _k = min(k, len(dist)-1) and then use _k rather than if/else, but either is fine ofc.

That is a much simpler solutions, thanks for pointing that out. I have made the change in afde81e

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.

Well, still fine 😄

@fkiraly fkiraly merged commit f22f218 into sktime:main Oct 27, 2023
46 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Fixes a known bug or removes unintended behavior module:annotation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Clasp Segmentation sometimes raises a ValueError
2 participants