-
Notifications
You must be signed in to change notification settings - Fork 87
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
Conversation
All PEP8 errors has been fixed, thanks ❤️ Comment last updated at Wed, 22 Jan 2025 21:10:21 |
/fix-pep8 |
Codecov ReportAttention: Patch coverage is
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. |
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 |
is_classification_task = input_data.task.task_type == TaskTypesEnum.classification | ||
is_regression_task = input_data.task.task_type == TaskTypesEnum.regression |
There was a problem hiding this comment.
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 тут некорректен, то стоит его обработать.
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Выше уже была такая проверка, можно переиспользовать переменную.
params = deepcopy(self.model_params) | ||
if params.get('early_stopping_rounds'): | ||
del params["early_stopping_rounds"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Возможно лучше вот так менять параметр, через self.params.update?
Это фиксирует то что он поменялся внутри модели.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Исправлено: исправил на self.params.update.
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')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Если код идентичен тому что выше - можно вынести в функцию.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Исправлено: вынес в отдельную функцию.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Не забывай только помечать в комментариях какие замечания исправлены.
This is a 🔨 code refactoring.
Summary
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 usingeval_set
due to the small dataset.