[timeseries] add model registry and fix presets typing #5100
Conversation
| @@ -244,7 +192,7 @@ def get_preset_models( | |||
| f"Keys of the `hyperparameters` dictionary must be strings or types, received {type(model)}." | |||
| ) | |||
|
|
|||
| for model_hps in hyperparameters[model]: | |||
| for model_hps in hyperparameter_dict[model]: | |||
| ag_args = model_hps.pop(constants.AG_ARGS, {}) | |||
There was a problem hiding this comment.
Thanks, the PR looks great! I guess we can also move this logic closer to the TimeSeriesTrainer at some point. IMO, there should be just one "preset_configs" file that contains
- equivalent of the
get_default_hpsmethod - preset configurations
Everything else feels like it should be a part of the trainer.
There was a problem hiding this comment.
this has already gotten a little big so I'll defer to a fast-follow PR.
f56b9da to
4ce1647
Compare
4ce1647 to
61b7365
Compare
7513af8 to
e1fe105
Compare
279ae34 to
d618c99
Compare
| _aliases = ["Qux"] | ||
| pass | ||
|
|
||
| class QuxModel(metaclass=ModelRegistry): |
There was a problem hiding this comment.
Should we use with pytest.raises(...) here to assert that the exception is raised?
There was a problem hiding this comment.
huh. I had done this, interesting the branch didn't sync...
| """Abstract base class for all time series models that take historical data as input and | ||
| make predictions for the forecast horizon. | ||
| """ | ||
|
|
||
| default_priority: int = 0 |
There was a problem hiding this comment.
Should we align the name of the priority var across tabular and timeseries?
Similarly, should we have ag_name and ag_key? This is what I use for the model registry logic in Tabular.
class CatBoostModel(AbstractModel):
ag_key = "CAT"
ag_name = "CatBoost"
ag_priority = 70There was a problem hiding this comment.
My answer again to the "should we align?" question is again "not without a compelling reason." Independently, I like ag_priority better so will use that. Analogues for key and name don't exist anyway so won't use those.
|
Job PR-5100-f514314 is done. |
Issue #, if available:
Description of changes:
ModelRegistryto time series. By default, all models which inherit fromAbstractTimeSeriesModelwill be registered.default_priorityto a field defined by models (WIP).By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.