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

Increase training and inference performance for GAK kernel #77

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

lejafar
Copy link

@lejafar lejafar commented Oct 21, 2018

Hi! I've been using the TimeSeriesSVC with the gak kernel for a while now and I've made some changes that increased the performance quite a bit, the most time consuming part in my case was computing the kernel and its there where I've made some changes

Changes that apply to training:

To compute the self-similarity, scikit-learn evaluates the callable kernel using a reference of X as the second argument, in the current implementation will check if the second argument is None which it isn't, hence it computes everything twice

Changes that apply to inference:

When doing prediction, scikit-learn evaluates the callable kernel for every point (X_fit) seen up until then, but the only thing libsvm really needs is the distance to its support vectors, hence we can omit computing the others.

Copy link
Member

@rtavenar rtavenar 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 your PR, this is a good idea indeed.

tslearn/svm.py Outdated Show resolved Hide resolved
tslearn/svm.py Show resolved Hide resolved
Copy link
Member

@rtavenar rtavenar 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 your PR, this is a good idea indeed.

Could you please answer my 2 questions so that I can make my opinion on these two technical points?

Thanks,
Romain

tslearn/svm.py Outdated Show resolved Hide resolved
tslearn/svm.py Show resolved Hide resolved
@rtavenar
Copy link
Member

Woops sorry for double posting my review...

@lejafar
Copy link
Author

lejafar commented Nov 6, 2018

@rtavenar anything else?

@rtavenar
Copy link
Member

Hi @lejafar

Sorry it took me so long to get back to you. Your suggestion is still welcome but the codebase has changed quite a lot since you posted it. Specifically, we are working on a future release in the dev branch.

So, my question is : would you be willing to close this PR and open a new one based on the current dev branch ?

The place where your hack could be inserted is probably around:
https://github.com/rtavenar/tslearn/blob/dev/tslearn/svm.py#L58-L60

Base automatically changed from master to main January 26, 2021 12:41
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.

2 participants