-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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] Rework of base series annotator API #6265
Conversation
ClaSP uses `fmt` to change the output format, so it has been kept as a attribute of the `ClaSPSegmentation` class.
As discussed in the last developer meeting, the approach using tags is showcased here: #6271 This allows to keep all "property"-like attributes in one, inspectable place. |
PS: if you migrate to tags, you need to merge the parts of #6271 as well that add tags to the registry. |
Warn that fmt and labels will be deprecated
No the index is not currently always monotonically increasing but it should be. In fact I think some of the new methods will break if index is not monotonically increasing. We need to enforce this. I have added deprecation warnings in clasp and the pyod annotator. I think those are the only two classes that are affected by deprecation. Currently, the deprecation message feels very brief:
Is this sufficient? Do we want to point a specific version when these arguments will be depracted? |
I would say sth like
using |
I have rewritten the deprecation warning. I have also removed the deprecation warning for If everyone else is happy are we ready to merge? |
Could someone please rerun the CI/CD pipeline? Looks like the docs jobs has failed due to running out of time to build. I don't think this as a result of changes introduced in this PR. |
Sure - rerunning |
Restarted - I also updated the deprecation messages and added release manager notes. Question, it seems we want to remove the |
This reverts commit fdaf7af.
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 we are ready to merge for release with 0.30.0.
@Alex-JG3, it would be great if you could check the updated warnings and release manager comments.
Yes, happy with those changes. About the deprecation warning for |
ok, let´s leave |
See sktime#3214 . #### What does this implement/fix? Explain your changes. <!-- A clear and concise description of what you have implemented. --> Reworks and refactors the `BaseSeriesAnnotator` class: * Removes the `fmt` and `label` attributes. * Adds the `learning_type` and `task` attributes. * Adds default `predict` and `transform` method. * Adds default `predict_points` and `predict_segments` methods. * Adds methods for converting between dense and sparse output formats. * Adds default methods for `predict` and `transform`. Also carries out a move of all existing annotation estimators to the new interface.
Reference Issues/PRs
See #3214 .
What does this implement/fix? Explain your changes.
Refactors the
BaseSeriesAnnotator
class:fmt
andlabel
attributes.learning_type
andtask
attributes.predict
andtransform
method.predict_points
andpredict_segments
methods.predict
andtransform
.Does your contribution introduce a new dependency? If yes, which one?
No.
What should a reviewer concentrate their feedback on?
Did you add any tests for the change?
Any other comments?
PR checklist
For all contributions
How to: add yourself to the all-contributors file in the
sktime
root directory (not theCONTRIBUTORS.md
). Common badges:code
- fixing a bug, or adding code logic.doc
- writing or improving documentation or docstrings.bug
- reporting or diagnosing a bug (get this pluscode
if you also fixed the bug in the PR).maintenance
- CI, test framework, release.See here for full badge reference
maintainers
tag - do this if you want to become the owner or maintainer of an estimator you added.See here for further details on the algorithm maintainer role.
For new estimators
docs/source/api_reference/taskname.rst
, follow the pattern.Examples
section.python_dependencies
tag and ensureddependency isolation, see the estimator dependencies guide.