Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Boostings implementations eval_set fix #1358

Merged
merged 12 commits into from
Jan 23, 2025

Conversation

dmitryglhf
Copy link
Collaborator

This is a 🔨 code refactoring.

Summary

  • Added new conditions for training boostings with eval_set.
  • Updated multi output for boostings

Resolved issue with test_preprocessing_through_api/test_categorical_target_processed_correctly. In cases where we tried to use LightGBM, XGBoost or CatBoost an error appeared when using eval_set due to the small dataset.

Copy link
Contributor

github-actions bot commented Jan 15, 2025

All PEP8 errors has been fixed, thanks ❤️

Comment last updated at Wed, 22 Jan 2025 21:10:21

@dmitryglhf
Copy link
Collaborator Author

/fix-pep8

Copy link

codecov bot commented Jan 15, 2025

Codecov Report

Attention: Patch coverage is 58.13953% with 18 lines in your changes missing coverage. Please review.

Project coverage is 80.13%. Comparing base (84f4ebb) to head (e0168a7).
Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
...mplementations/models/boostings_implementations.py 58.53% 17 Missing ⚠️
fedot/core/operations/evaluation/boostings.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1358      +/-   ##
==========================================
- Coverage   80.16%   80.13%   -0.03%     
==========================================
  Files         146      146              
  Lines       10492    10512      +20     
==========================================
+ Hits         8411     8424      +13     
- Misses       2081     2088       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pep8speaks
Copy link

pep8speaks commented Jan 16, 2025

Hello @dmitryglhf! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2025-01-22 18:09:27 UTC

@dmitryglhf dmitryglhf requested a review from nicl-nno January 17, 2025 17:03
Comment on lines 44 to 45
is_classification_task = input_data.task.task_type == TaskTypesEnum.classification
is_regression_task = input_data.task.task_type == TaskTypesEnum.regression
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

А task_type == ts_forecasting до сюда не может дойти? В любом случае, если случай с обоими false тут некорректен, то стоит его обработать.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Исправлено: добавлена проверка на уровень выше в модуле boostings.py

del params["early_stopping_rounds"]

# Create model with updated params
if input_data.task.task_type == TaskTypesEnum.classification:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Выше уже была такая проверка, можно переиспользовать переменную.

Comment on lines 71 to 73
params = deepcopy(self.model_params)
if params.get('early_stopping_rounds'):
del params["early_stopping_rounds"]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Возможно лучше вот так менять параметр, через self.params.update?

https://github.com/aimclub/FEDOT/blob/master/fedot/core/operations/evaluation/operation_implementations/data_operations/ts_transformations.py#L131

Это фиксирует то что он поменялся внутри модели.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Исправлено: исправил на self.params.update.

Comment on lines 179 to 187
train_input, eval_input = train_test_data_setup(input_data)

# Conditions for training with eval_set
is_classification_task = input_data.task.task_type == TaskTypesEnum.classification
is_regression_task = input_data.task.task_type == TaskTypesEnum.regression
all_classes_present_in_eval = (
np.unique(np.array(train_input.target)) in np.unique(np.array(eval_input.target))
)
is_using_eval_set = bool(self.params.get('use_eval_set'))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Если код идентичен тому что выше - можно вынести в функцию.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Исправлено: вынес в отдельную функцию.

@dmitryglhf dmitryglhf requested a review from nicl-nno January 23, 2025 09:48
Copy link
Collaborator

@nicl-nno nicl-nno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Не забывай только помечать в комментариях какие замечания исправлены.

@dmitryglhf dmitryglhf merged commit 6dd93b2 into master Jan 23, 2025
10 checks passed
@dmitryglhf dmitryglhf deleted the boostings-implementations-eval-set-fix branch January 23, 2025 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants