Conversation
| @property | ||
| def is_fitted(self): | ||
| """Has `fit` been called?""" | ||
| """Wheter `fit` been called.""" |
There was a problem hiding this comment.
| """Wheter `fit` been called.""" | |
| """Whether `fit` has been called.""" |
|
@mloning do you want me to add any unit tests for I started to look through the tests in test_all_estimators and could work up tests that make sense for |
|
Yes, quite happy with this - great idea, and it gives us a place to control functionality that all objects get. This is out of date with the main branch, but in-principle I'm happy to approve. |
|
Hi @RNKuhns, yes adding unit tests would be a good idea. Do we want to test all public objects (not just estimators) inherit from the |
|
@mlonging, I had been thinking about something along those lines. Beyond that is there anything else I should look to create a unit test for? I'll try to get this knocked out this weekend. |
|
In that case, I suggest to refactor If we enforce these tests, we also need to modify a few existing classes which currently do not inherit from the I think we would want to take another look at the tags again at some point in the future, but that seems to be a separate PR. |
| scikit-learn to make sure it does not interfere with scikit-learn's tag | ||
| interface when inheriting from scikit-learn classes. Sktime's | ||
| estimator tags are class rather than object attribute as in scikit-learn. | ||
| """ |
There was a problem hiding this comment.
this should properly describe that this implements a kind of inheritance, no?
As far as I understand, if multiple estimators in the inheritance tree have the same tag, the descendant tag overrides the parent tag.
This is important to document, perhaps with an example of what tags are returned.
There was a problem hiding this comment.
I'd also like to rename the _all_tags method to get_tags and make it public if that doesn't interfere with scikit-learn's tags. But perhaps changes to the tag system are best addressed in a separate PR.
There was a problem hiding this comment.
minor comment: I was wondering, upon a vague feeling, whether this could be just solved by using inheritance smartly? Namely, if we were just looking at the dictionary, and its keys as attribute names, this is precisely inheritance of attributes.
Inside the list, the name for this pattern is inheritance of nested attributes - it doesn't seem python supports this natively. A quick google search indicates it's an occasional topic in various other programming languages as well (mostly ending in the conclusion that the language doesn't support it).
|
In the next step, I would like to add a |
|
Thanks for pinging me @fkiraly. I had a look at |
Well, in this case one would probably say "implement" rather than overwrite/override since it's abstract. But yes, exactly!
Hm, this looks really weird - the model is wrapped in the |
Nope - I share your confusion here. Further investigation is required to get to the bottom of this. |
Rework of PR #1099 which is in turn a rework of PR #891 (by @RNKuhns), based on discussion recorded in #981 on tags. Adds a `BaseObject` class which adds dynamic tag functionality. `BaseObject` contains `BaseEstimator` functionality sans `fit` state change related interfaces. The new functionality replaces the inconsistent mixture of old interface points (`all_tags`, `has_tag`) in all downstream dependencies. post-PR, tags work like this: * each class will have "class tags", stored in the content of the class variable `_tags: dict`. Individual class tags are considered to override each other as per normal nested inheritance. E.g., if a parent class and the class have different values for a class tag, the younger class' class tag value counts. * each object can, in addition, have dynamic tags, in the object variable `_tags_dynamic: dict`. These are expected to be initialized in the constructor, for instance in cases where behaviour described by tags depends on components or hyper-parameters. Dynamic tags are always considered to override the class tags - e.g., if the object's class or a parent class have a static tag of the same name, the dynamic tag's value overrides any class tag's value. The new tag related methods make use of this inheritance logic are as follows. class methods: * `get_class_tags()` collects all class tags and their values and returns them as a `dict` (keys = tag names, values = tag values). It overrides values precisely as described above, in the return dictionary. This replaces the old `_all_tags`. * `get_class_tag(tag_name, tag_value_default=None)` retrieves the value of an individual tag with name `tag_name`, and optionally allows to specify a default value if the tag is not found. The inheritance/override logic is the same as in `get_class_tags`. object methods: * `get_tags()` collects all class tags and dynamic tags and their values and returns them as a `dict` (keys = tag names, values = tag values). It overrides values precisely as described above, in the return dictionary. * `get_tag(tag_name, tag_value_default=None)` retrieves the value of an individual tag with name `tag_name`, and optionally allows to specify a default value if the tag is not found. The inheritance/override logic is the same as in `get_tags`. * `set_tags(**tag_dict)` sets dynamic tags according to keys (=tag names) and values (= tag values) in the argument dictionary. It is expected to be used in constructors typically. * `mirror_tags(estimator, tag_set=None)` is a shorthand version of `set_tags`, where it sets tags according to their `get_tag` values in `estimator`. The tags that are mirrored/copied in this way can be restricted by specifying a list of tag names in `tag_set`, otherwise all tags are copied over. The mirroring is by-reference and not by-value (if the type allows this), but tags are not expected to be mutated.
|
superseded by the now merged #1134 |
Reference Issues/PRs
#877
What does this implement/fix? Explain your changes.
Creates Sktime BaseObject class that inherits from scikit-learn's BaseEstimator and only adds the method for returning estimator tags.
Does your contribution introduce a new dependency? If yes, which one?
No
What should a reviewer concentrate their feedback on?
Changes in _base.py. The changes in _meta.py are just to comply with pydocstyle checks (so all of base is compliant).
Any other comments?
Still need to set up the unit testing (want to apply applicable the unit tests that are currently applied to BaseEstimator except those dealing with fit functionality).
PR checklist
For all contributions
For new estimators