-
Notifications
You must be signed in to change notification settings - Fork 152
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] ADD SAST transformer and SASTClassifier #958
Conversation
- SAST: a subsequence based transformation for time series - SASTClassifier: as helper classifier to build a time series classifier based on the SAST transformation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Thank you for contributing to
|
awesome, thanks @frankl1, we will take a look next week. |
Hi all, I could not figure out the cause of the failing test. It doesn't seem to be related to my changes. Since I couldn't manage to reproduce it locally, I wonder if there is a way to get more information on the error from the job log. Thanks |
hi @frankl1 so sometimes the CI crashes when too much is running at once. Its passed all now. I'm away a couple of days but we will review it asap, thanks for the contribution, really pleased you made the effort. Did you join our slack? Be good to catch up |
Thanks for the reply @TonyBagnall. It is my pleasure to contribute to this great project and I plan to contribute more in the future. I haven't joined the Slack, will catch up. |
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.
I'd rather not introduce a whole new category, could this go in the shapelet package?
Hi @TonyBagnall , Thanks for reviewing the PR. I don't mind using the shapelet package for this since the method is able to learn shapelets. I will make the update today. |
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, I'm jumping in on the review as I've seen the paper and wanted to look at the code !
If you don't mind, I think the following things could be changed.
Additionally, it would be nice to have some unit tests for the transformer and classifier.
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.
LGTM, thanks
Thanks for the contribution @frankl1, I didnt see anything holding this back from going in on my look through. I think some tests would be good to add, but won't push for them in this PR. Are there any more changes you want to make for this? If not, you can ping @baraline to re-review and we can move it forward I think if they agree. Sorry for the delay! |
Hi, I am sorry for the late reply, I was on vacation. There are no changes I would like to add to this @MatthewMiddlehurst. I will work on the multivariate version and make a new PR. Thanks for merging with the main branch. Regard |
What does this implement/fix? Explain your changes.
Add implementation of the SAST ans SASTClassifier:
Associated paper
Does your contribution introduce a new dependency? If yes, which one?
matplotlib
PR checklist
For all contributions
For new estimators and functions