-
-
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] Remove attrs dependency #5296
Conversation
* Attrs is replaced with dataclasses
* Attrs is replaces with dataclasses
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! This seems to remove the unnecessary dependency completely.
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.
We should also remove it from the all_extras
dependency set.
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.
there's one more, in all_extras_pandas2
... just do a repo search for attrs
, and let's check if it is entirely gone.
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.
Looks good now!
Looking at this, I think the methods are not conformant with the usual expectation that they return only and exactly the Fitted parameters, if any, should get exposed via |
* origin/split-ci: (21 commits) fixed typo add back checkout change names switch to path filters edit format update action version run all tests manually and on schedule akipped workflows on main added missing installation of test dependencies added new extra for testing run base tests skip unnecessary checkout step renamed workflow file run component tests test component specific functionalities test base functionalities [ENH] Remove attrs dependency (sktime#5296) [ENH] add proper `inverse_transform` to `STLTransformer` (sktime#5300) [DOC] add `blog` badge for `fkiraly`, for ODSC blog post (sktime#5291) [BUG] in `Imputer`, fix `y` not being passed in `method="forecaster"` (sktime#5287) ...
…recasting * origin/split-ci: (21 commits) fixed typo add back checkout change names switch to path filters edit format update action version run all tests manually and on schedule akipped workflows on main added missing installation of test dependencies added new extra for testing run base tests skip unnecessary checkout step renamed workflow file run component tests test component specific functionalities test base functionalities [ENH] Remove attrs dependency (sktime#5296) [ENH] add proper `inverse_transform` to `STLTransformer` (sktime#5300) [DOC] add `blog` badge for `fkiraly`, for ODSC blog post (sktime#5291) [BUG] in `Imputer`, fix `y` not being passed in `method="forecaster"` (sktime#5287) ...
Reference Issues/PRs
Closes #5290.
What does this implement/fix? Explain your changes.
attrs
withdataclasses
in theigts
andggs
modules of theannotation
module.Does your contribution introduce a new dependency? If yes, which one?
No, but it removes
attrs
as a dependency for theannotation
module.What should a reviewer concentrate their feedback on?
I would like feedback on the
get_params
methods for theInformationGainSegmentation
andGreedyGaussianSegmentation
classes. Usingattrs
, one can easily filter attributes whereinit
isFalse
. That is not so easy withdataclasses
so I filtered the unwanted attributes manually. We could have tags in theGSS
andIGTS
classes called something like__attributes_to_ignore__
which would be a list of attributes to ignore in theget_params
methods. But, since the pattern used forGGS
andIGTS
is likely going to change soon, I didn't think it necessary to use the tags approach.Did you add any tests for the change?
No, but I changed the
test_ggs.py
andtest_igts.py
files to reflect the refactoring inggs.py
andigts.py
respectively.Any other comments?
Can
attrs
be removed from theall_extras
andall_extras_pandas2
lists of dependencies inpyproject.toml
?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
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.