Bagging method implementation to FEDOT #1005
Conversation
aPovidlo
added
in progress
aPovidlo
changed the title
Codecov Report
@@ Coverage Diff @@
## master #1005 +/- ##
==========================================
+ Coverage 79.70% 88.97% +9.26%
==========================================
Files 141 133 -8
Lines 9851 9433 -418
==========================================
+ Hits 7852 8393 +541
+ Misses 1999 1040 -959
|
...e/operations/evaluation/operation_implementations/models/ensemble_implementations/bagging.py
Outdated
Show resolved
Hide resolved
...e/operations/evaluation/operation_implementations/models/ensemble_implementations/bagging.py
Outdated
Show resolved
Hide resolved
.../operations/evaluation/operation_implementations/models/ensemble_implementations/combiner.py
Outdated
Show resolved
Hide resolved
.../operations/evaluation/operation_implementations/models/ensemble_implementations/combiner.py
Outdated
Show resolved
Hide resolved
.../operations/evaluation/operation_implementations/models/ensemble_implementations/combiner.py
Outdated
Show resolved
Hide resolved
.../operations/evaluation/operation_implementations/models/ensemble_implementations/ensemble.py
Outdated
Show resolved
Hide resolved
.../operations/evaluation/operation_implementations/models/ensemble_implementations/ensemble.py
Outdated
Show resolved
Hide resolved
|
|
||
| def __init__(self, operation_type, params: Optional[OperationParameters] = None): | ||
| super().__init__(operation_type, params) | ||
| self._bagging_params, self._model_params = self._set_operation_params(operation_type, params) |
There was a problem hiding this comment.
Эту строчку спокойно можно перенести в базовый класс и можно убрать аргументы, т.к. ты в EvaluationStrategy инициализируешь параметры и тип операции
There was a problem hiding this comment.
Да и следущую тоже. _convert_to_operation можно перенести в базовый класс, а вместо BaggingClassifier или BaggingRegressor можно написать в суперклассе что-то вроде bag_type(**self.bagging), где get_bagбудет BaggingClassifier или BaggingRegressor
Думаю сработает
There was a problem hiding this comment.
Не совсем тебя понял. bag_type() в классе предка, а вызывать его из класса родителя? А get_bag откдуа?
И в итоге, не вернулись ли туда, от куда начали разбитие на два класса? Уж лучше было когда все в одном
There was a problem hiding this comment.
Думаю, что _convert_to_operation пока нужно оставить в каждом из классе
There was a problem hiding this comment.
class SkLearnBaggingStrategy(EvaluationStrategy, ABC):
.....
def __init__():
....
self.bag_type = None
....
def _convert_to_operation(self, operation_type: str):
if operation_type in self.__operations_by_types.keys():
if self._model_params:
self._bagging_params['base_estimator'] = self.__operations_by_types[operation_type](
**self._model_params)
else:
self._bagging_params['base_estimator'] = self.__operations_by_types[operation_type]()
return self.bag_type(**self._bagging_params)
class SkLearnBaggingClassificationStrategy(SkLearnBaggingStrategy):
....
def __init__():
....
self.bag_type = BaggingClassifier
....
```С регрессией также. Не уверен, что это лучшее решение, но сокращает код в 2 раза
There was a problem hiding this comment.
@valer1435 перенес в базовый класс
| if params is None: | ||
| params = get_default_params(operation_type) | ||
| elif isinstance(params, dict): | ||
| params = OperationParameters.from_operation_type(operation_type, **params) |
There was a problem hiding this comment.
На этапе вызова этой функции у нас уже должны по идее инициализироваться параметры. Зачем из заново объявлять?
There was a problem hiding this comment.
Для тестов, в которых вызывается необходимый класс бэггинга, и возможности передачи параметров.
There was a problem hiding this comment.
Думаю, все-таки перепишу тесты через PipelineBuilder и уберу этот метод
There was a problem hiding this comment.
Вспомнил. У меня ранее падал тест test_api_print_info_correct. В других реализациях параметры использовались только на уровне fit, поэтому в них нужды в __init__ не было. А так как сейчас есть необходимость, а в инициализацию params приходят равные None они и вызывают ошибку. Поэтому решил добавить функцию, которая их бы добавляла. Поэтому нет, на этапе вызоыва этой функции у нас еще нет иницилизированных параметров.
There was a problem hiding this comment.
Идею понял, но есть несколько предложений:
- убрать @static и записывать bagging_params, model_params в поля класса
- Сделать вызов этого метода до инициализации конструктора базового класса и сделать так, чтобы метод возвращал params. params потом уже передавать в конструктор. Нужно, чтобы не было 2 разных объекта параметров. Сейчас именно так
There was a problem hiding this comment.
@valer1435 Изменил
test/unit/data_operations/test_data_operations_implementations.py
Outdated
Show resolved
Hide resolved
| if params is None: | ||
| params = get_default_params(operation_type) | ||
| elif isinstance(params, dict): | ||
| params = OperationParameters.from_operation_type(operation_type, **params) |
There was a problem hiding this comment.
Идею понял, но есть несколько предложений:
- убрать @static и записывать bagging_params, model_params в поля класса
- Сделать вызов этого метода до инициализации конструктора базового класса и сделать так, чтобы метод возвращал params. params потом уже передавать в конструктор. Нужно, чтобы не было 2 разных объекта параметров. Сейчас именно так
|
|
||
| return self._convert_to_output(prediction, predict_data) | ||
|
|
||
| def predict_proba(self, trained_operation, predict_data: InputData) -> OutputData: |
There was a problem hiding this comment.
Имплементации разве сами не подхватывают predict_proba, когда нужно? Посмотрел на метод predict в SkLearnClassificationStrategy, там эта проблема уже решается. Так что это должен быть бесполезный метод
| raise ValueError(f'Impossible to create bagging operation for {operation_type}') | ||
|
|
||
| def fit(self, train_data: InputData): | ||
| return super().fit(train_data) |
There was a problem hiding this comment.
Это работает по-другому - ты просто можешь не реализовывать методы fit и predict, так как они есть в родительском классе. А по иерархии наследования то, что является объектом дочернего класса, является также носителем свойств родительского класса -> все методы сохраняются
There was a problem hiding this comment.
Да, в курсе, но, как по мне, так намного лучше читается сам код и легче будет потом разобраться что к чему
There was a problem hiding this comment.
Дублировать так реализации методов базового класса действительно нет особого смысла, ето анти-паттерн.
There was a problem hiding this comment.
@valer1435 @nicl-nno уберу в следующем коммите
| } | ||
| }, | ||
| 'bag_xgboost': { | ||
| 'bagging_params': { |
There was a problem hiding this comment.
Вообще, чтобы сделать вложенные параметры для hyperopt, надо помучаться. Ты уверен, что тюнинг будет работать? В том смысле, что гиперпараметры будут изменяться? Возможно стоит избавиться от model_params и bagging_params тут и разделять их как-то по-другому
There was a problem hiding this comment.
Правда тут не то, чтобы параметры вложенные, я больше про разделение
There was a problem hiding this comment.
С тюнингом пока все сложно в плане реализации. Про использование вложенных параметров для hyperopt пока еще не успел посмотреть, поэтому не знаю работает ли тюнинг, это сейчас и выясняю. Изначально у меня была немного другая идея по оптимизации базовых моделей в бэггинге. Из-за временной дороговизны операции, думаю, что будет более рационально использовать найденные на этапе композирования и тюнинг модели, которые после можно обернуть в бэггинг и посмотреть улучшает ли он результат.
Пример, во время композирования и тюнинга мы нашли оптимальный пайплайн 'lgbm' {с найденными оптимальными параметрами} и в качестве базовой моделей бэггинга используем эту модель с найденными параметрами.
There was a problem hiding this comment.
Однако, пока все еще не понимаю, как их использовать если у нас какая-то более разветвленная структура пайплайна получилась
There was a problem hiding this comment.
@valer1435 это остается пока все еще актуальным. Сейчас начал проводить эксперименты для поиска оптимальных параметров. Возможно, в будущем получится обойтись и без тюнинга
aPovidlo
force-pushed
the
ag_exp_ensembles
branch
from
d0dff12
to
a4df3a4
Compare
|
Hello @aPovidlo! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2023-09-21 15:22:57 UTC |
| mutable_node = choice(mutable_candidates) | ||
|
|
||
| replaced_node_index = pipeline.nodes.index(mutable_node) | ||
| replaced_node = pipeline.nodes[replaced_node_index] |
There was a problem hiding this comment.
Вроде тут replaced_node == mutable_node/ Смысл выбирать replaced_node?
There was a problem hiding this comment.
@valer1435 возможно, попробую тогда убрать
| bagging_node = 'bag_' + mutable_node.name | ||
| new_node = PipelineNode(operation_type=bagging_node, params=new_params) | ||
|
|
||
| new_node = PipelineBuilder().add_node(bag_model, params=bag_params).build().nodes[0] |
There was a problem hiding this comment.
Лучше создать просто PipelineNode без пайплайна.
There was a problem hiding this comment.
@valer1435 не работает так, потому что он параметры не принимает
There was a problem hiding this comment.
И правда. Но можно сделать так
new_node=PipelineNode('bag_node)
new_node.parameters = bag_params
Мне кажется выглядит логичнее и проще
| @@ -85,6 +85,9 @@ def _set_operation_params(self, operation_type, params): | |||
| params = get_default_params(operation_type) | |||
| elif isinstance(params, dict): | |||
| params = OperationParameters.from_operation_type(operation_type, **params) | |||
| elif isinstance(params, OperationParameters): | |||
| if params.get('model_params') or params.get('bagging_params'): | |||
| params = OperationParameters.from_operation_type(operation_type, **(params.to_dict())) | |||
There was a problem hiding this comment.
Если и так приходит OperationParams, зачем его создавать заново?
There was a problem hiding this comment.
@valer1435 при мутации мы передаем параметры из ноды модели уже в OperationParameters(...). Но в нем отсутствует bagging_params, поэтому необходимо подтянуть дефолтные
There was a problem hiding this comment.
Поясни пж смысл условия наличия параметров модели ИЛИ наличия бэггинг параметров?
There was a problem hiding this comment.
Ну если где-то они будут отсутствовать или в model_params, или в bagging_params, или в обеих, то взять дефолтные
There was a problem hiding this comment.
Поправь, если не прав, но тогда условие должно выглядеть так: if not (params.get('model_params') and params.get('bagging_params')): ?
aPovidlo
force-pushed
the
ag_exp_ensembles
branch
from
8648bce
to
2a12f83
Compare
|
|
| @@ -85,6 +85,9 @@ def _set_operation_params(self, operation_type, params): | |||
| params = get_default_params(operation_type) | |||
| elif isinstance(params, dict): | |||
| params = OperationParameters.from_operation_type(operation_type, **params) | |||
| elif isinstance(params, OperationParameters): | |||
| if params.get('model_params') or params.get('bagging_params'): | |||
| params = OperationParameters.from_operation_type(operation_type, **(params.to_dict())) | |||
There was a problem hiding this comment.
Поясни пж смысл условия наличия параметров модели ИЛИ наличия бэггинг параметров?
aPovidlo
force-pushed
the
ag_exp_ensembles
branch
from
913fb03
to
d4da38b
Compare
Refactoring into SkLearn bagging method Reviewers fixes, separates to classes and tests fixes ensemble implementation first step ensemble, stacking and bagging implementation methods were integrated to Fedot Refactoring, added splitter, attempting to integrate ensemble selection method minor changes to experiment Refactoring into SkLearn bagging method Added bagging models and edited tests
fixes after rebase Fixes after rebase, adding bagging mutation and test pep8 bot review's fix Editing params setting and fix test test fix added bagging_allowed tag for models deleting unnecessary method
…rameters_change_mutation
aPovidlo
force-pushed
the
ag_exp_ensembles
branch
from
d4da38b
to
405a3f5
Compare
|
Hello @aPovidlo! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2023-10-19 09:18:20 UTC |
Taking out Catboost model from SkLearnEvaluationStrategy into separate Boosting class strategy. New features for CatBoost: Converting data into Pool; Fitting with eval_set, early_stopping_rounds and best_model; Defining categoricat features for model; Ploting features importances; Saving/Loading model (required for implementation in Bagging method implementation to FEDOT #1005); Updated search space. Updates in data, preprocessing and data_split: Storing not encoded categorical features (into OHE or LE) and using it where necessary; Storing positions for different types of data (numeric, category and encoded); Storing features names of dataset.
|
Closed as obsolete |
Adding an ensemble (stacking and bagging) operations to FEDOTUPD: The main task was reformulated and divided into small tasks. In this PR is solving one from this tasks, more specifically bagging implementation.
Implementation:
bagging method from sklearn to FEDOT
multi-layer stack ensembling + n-repeated k-fold bagging method for boosting methods
Minor changes: Added ExtraTrees models to FEDOT