-
Notifications
You must be signed in to change notification settings - Fork 207
[ENH]Use n_jobs parameter in KNeighborsTimeSeriesClassifier. #2687
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
Conversation
Thank you for contributing to
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, could you verify output is the same from before and show that the mutlithreading does make the classifier faster.
In #2478, I've put some benchmarks. These are the entries on three data sets with 1 or 8 cores with only Aeon's builtin DTW:
For the small GunPoint data set, overhead is larger than gains from parallelization. I expect that for ED, data sets would need to be larger to get a performance increase (if that's even possible) compared to DTW. Equivalence is tested in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Could you edit the n_jobs
docstring and consider implementing the backend parameter found in other threaded classifiers. Don't have to do the second but would be a nice addition I think.
LGTM. Question is still open on whether this is the best way to do this for KNN, but it is much better than nothing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes thanks for this
Reference Issues/PRs
Fixes #2478. See also #2545 and #2578.
What does this implement/fix? Explain your changes.
Uses n_jobs parameter in
_predict
andpredict_proba
ofKNeighborsTimeSeriesClassifier
. Parallelization is done in these methods instead of_kneighbors
to potentially allow speedup through upper bounding the distance.PR checklist
For all contributions