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
Add Clasp for time series segmentation (CIKM'21 publication) #1352
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
@fkiraly two things I forgot to mention in my last commit :)
|
Apologies for the wait - to speed things up, I've resolved some merge conflicts outside the remit of the PR, in the Since it's partly my fault letting you wait that things got out of sync. Kindly check whether all is fine and feel free to revert. |
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 think the interfaces are now good - there are a few things I'd like to iterate on, but let's do that once the estimator is in.
However, we need to improve a few docstrings before this can be merged, or the automated documentation will not render properly.
I'm adding the comments below, please action those and then we can merge.
@fkiraly I have addresses your comments. I appreciate your comments and the time you invest in reviewing. I also love sktime and its idea, which is why I am very willing to contribute. But please consider: you are starting to expect a level of industry-grade polishing of the code, which is barely present in the other module in sktime. But was for sure not present at the start of their development. I spent some work days on this pull request now. And as of now, I am a bit unsure, if I will push our research to sktime in future times, as it is so time demanding to contribute. Thus, there should be some debate on how to lower the burden of contributing to sktime. |
Hi Patrick, don't let Franz put you off, it's his way. I'll take a look when I'm back, off for half term |
@patrickzib, what I've said above is that I'm happy to merge as long as the tests are! Sorry for the back-and-forth, that's related to the fact that this is a new "type" of estimator, and we've been consolidating design decisions together with the algorithm itself. At the same time, I also need to think about our users, for whom clear usage and well-documented code is of course better - it's a difficult balance to strike sometimes. I'd say that balance is somewhere in the middle between "research-grade" and "industry-grade", so if both of us grow equally frustrated (coming from different directions) perfection is achieved. We also wanted to write you an email about contributions, but since I read your frustration, perhaps it's better to say some of this in public: we really appreciate your dedication and your work! We were also thinking of asking you to take the lead on some annotation, changepoint, or segmentation related area if you would like 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.
happy
@fkiraly thank you :) |
Reference Issues/PRs
#1346
What does this implement/fix? Explain your changes.
This request implements ClaSP, a novel time series segmentation method published at CIKM '21.
The segmentation is implemented as an annotator, and the profile in ClaSP is implemented as a SeriesToSeriesTransformer.