Conversation
|
@fkiraly Removing |
|
hm, interesting - I would introduce a tag called The second line is done in the base class, the first (addition of tag) in the concrete class. |
|
I've suggested addition of this as a tag in #1013. Once that is agreed and merged, you could set the |
|
hm, I guess a temporary solution is to leave the conversion inside, as you did. Opinions, @mloning? |
fkiraly
left a comment
There was a problem hiding this comment.
happy with this to be merged as is, assuming we'll deal later with the enforece_index_type tag
aiwalter
left a comment
There was a problem hiding this comment.
I left one comment @thayeylolu
|
Hello @aiwalter . I would like to suggest leaving the univariate only tag as True for the meantime. then, when we resolve the tag issues, we can go ahead and set it as False |
Adds some default tag values in `BaseForecaster` - these are automatically inherited via `_all_tags`. Also adds two new tags that I anticipate we will need: * `"X-y-must-have-same-index"` - do `X` and `y` need to have the same index? * `"enforce-index-type"` argument to `check_X`/`check_y` if needed - this has to be a tag now, see the problem in #1005 . I've also added these new tags to the extension template. This PR also: * corrects a minor bug: mis-spelled tags in the extension template * adds the new tags to the allowed tags in tests * removes the requirement for tags to be boolean
Reference : #955
We need to slowly re-factor all forecasters to be compliant with the new interface (see PR #912). i.e., no overrides of fit etc and descendant classes only implementing core logic, while the BaseForecaster implements check etc logic.
Currently this is running due to various downwards compatibility workarounds, which should be removed once all forecasters are ready (in "no-override" state)
The forecasters that still need a downstream refactor are: