Add support for included_model_types #3239
Conversation
| missing_models = set(included_model_types) - set(included_models) | ||
| if len(missing_models) > 0: | ||
| logger.warning( | ||
| f"The following models: {missing_models} are not present in model list specified by the user: {models.keys() if isinstance(models, dict) else models}. Will ignore." |
There was a problem hiding this comment.
"The models types {missing_models} are not present in the model list specified by the user and will be ignored:"
| @pytest.mark.parametrize( | ||
| "models,included_model_types,excluded_model_types,expected_answer", | ||
| [ | ||
| ({"dummy": {}}, ["dummy"], None, {"dummy": {}}), | ||
| ({"dummy": {"dummy": 1}}, ["dummy"], None, {"dummy": {"dummy": 1}}), | ||
| ({"dummy": {}}, ["foo"], None, {}), | ||
| ({"dummy": {}, "foo": {}}, ["foo"], None, {"foo": {}}), | ||
| ({}, ["foo"], None, {}), | ||
| ({"dummy": {}}, None, ["dummy"], {}), | ||
| ({"dummy": {"dummy": 1}}, None, ["dummy"], {}), | ||
| ({"dummy": {}}, None, ["foo"], {"dummy": {}}), | ||
| ({"dummy": {}, "foo": {}}, None, ["foo"], {"dummy": {}}), | ||
| ({}, None, ["foo"], {}), | ||
| ({"dummy": {}}, ["dummy"], ["dummy"], {"dummy": {}}), | ||
| ({"dummy": {"dummy": 1}}, ["dummy"], ["dummy"], {"dummy": {"dummy": 1}}), | ||
| ({"dummy": {}}, ["foo"], ["dummy"], {}), | ||
| ({"dummy": {}, "foo": {}}, ["foo"], ["dummy"], {"foo": {}}), | ||
| ({}, ["foo"], ["foo"], {}), | ||
| (["dummy"], ["dummy"], None, ["dummy"]), | ||
| (["dummy"], ["foo"], None, []), | ||
| (["dummy", "foo"], ["foo"], None, ["foo"]), | ||
| ([], ["foo"], None, []), | ||
| (["dummy"], None, ["foo"], ["dummy"]), | ||
| ([], None, ["foo"], []), | ||
| (["dummy"], None, ["dummy"], []), | ||
| (["dummy", "foo"], None, ["dummy"], ["foo"]), | ||
| ], | ||
| ) |
There was a problem hiding this comment.
It may be easier to have 1 line per argument for each test with proper use of whitespace, otherwise it is very hard to verify correctness by simply looking at this block.
|
|
||
| @staticmethod | ||
| def include_models( | ||
| models: Union[Dict[str, Any], List[str]], included_model_types: List[str] |
There was a problem hiding this comment.
Why support both list and dict input for models? When is list ever used? Ditto for exclude_models
There was a problem hiding this comment.
I had a look at TimeSeries implementation of excluded_model_types and it was using list. We could of course change how TimeSeries is implementing the logic but I feel it could be a valid use case while not adding too much difficulty in maintaining
There was a problem hiding this comment.
+1 on list support: include=['RF', 'NN'] is a valid use case.
|
Job PR-3239-91f10e1 is done. |
|
Job PR-3239-01ced30 is done. |
Issue #, if available:
#1458
Description of changes:
included_model_typessupportBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.