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] ClaSP: Adds parallelization for distance computations and numbarize function calls #1692

Merged
merged 9 commits into from
Jun 21, 2024

Conversation

patrickzib
Copy link
Contributor

@patrickzib patrickzib commented Jun 17, 2024

This is mostly a performance enhancement of the ClaSP implementation by:

  • Parallelizing ClaSP distance computations
  • Numbarizing methods.
  • exposing n_jobs to ClaSP-Constructor class
  • convert ClaSPTransformer to BaseSeriesTransformer

@patrickzib patrickzib added enhancement New feature, improvement request or other non-bug code enhancement segmentation Segmentation package labels Jun 17, 2024
@patrickzib patrickzib requested a review from TonyBagnall June 17, 2024 07:46
@patrickzib patrickzib self-assigned this Jun 17, 2024
@aeon-actions-bot
Copy link
Contributor

Thank you for contributing to aeon

I would have added the following labels to this PR based on the changes made: [ $\color{#41A8F6}{\textsf{transformations}}$ ], however some package labels are already present.

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.

PR CI actions

These checkboxes will add labels to enable/disable CI functionality for this PR. This may not take effect immediately, and a new commit may be required to run the new configuration.

  • Run pre-commit checks for all files
  • Run all pytest tests and configurations
  • Run all notebook example tests
  • Run numba-disabled codecov tests
  • Stop automatic pre-commit fixes (always disabled for drafts)

@TonyBagnall
Copy link
Contributor

maybe convert to a BaseSeriesTransformer at the same time (#1618)?

@patrickzib
Copy link
Contributor Author

patrickzib commented Jun 18, 2024

I was thinking about removing the Transformer all together and merge it into ClaSP Segmenter? :) Or should I convert it?

@patrickzib
Copy link
Contributor Author

I was thinking about removing the Transformer all together and merge it into ClaSP Segmenter? :) Or should I convert it?

I meant merge the Transformer with the Segmenter, and remove the Transformer

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.

Can either convert it or merge into the segmenter. Up to you really, if you think the transform has potential for usage elsewhere may be nice to keep.

aeon/transformations/clasp.py Outdated Show resolved Hide resolved
aeon/transformations/clasp.py Outdated Show resolved Hide resolved
aeon/transformations/clasp.py Outdated Show resolved Hide resolved
@TonyBagnall
Copy link
Contributor

I was thinking about removing the Transformer all together and merge it into ClaSP Segmenter? :) Or should I convert it?

I think remove the transformer and just merge it into a clasp segmenter is best way forward

@patrickzib patrickzib requested a review from aiwalter as a code owner June 20, 2024 13:56
@patrickzib patrickzib removed the request for review from aiwalter June 20, 2024 14:03
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@patrickzib
Copy link
Contributor Author

Thank you @MatthewMiddlehurst and @TonyBagnall

I set n_jobs to 1 and have converted the BaseTransformer to a BaseSeriesTransformer for now.

Felt like merging everything into one module is also not the best option for now. Might re-consider this in the future :)

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.

LGTM

@patrickzib patrickzib merged commit 2b48e53 into main Jun 21, 2024
14 checks passed
@patrickzib patrickzib deleted the clasp branch June 21, 2024 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature, improvement request or other non-bug code enhancement segmentation Segmentation package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants