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

Add XGBoost integration #65

Merged
merged 16 commits into from Feb 8, 2024

Conversation

buruzaemon
Copy link
Contributor

Motivation

Description of the changes

  • Migrated optuna/integration/xgboost.py and its corresponding test from tests/integration_tests/test_xgboost.py to optuna_integration/xgboost.py and tests/test_xgboost.py, respectively. Additional related changes were made to update/correct the documentation as well.

@nabenabe0928
Copy link
Collaborator

@y0z Could you review this PR?

Copy link
Member

@y0z y0z left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution.
I leave a review comment.

optuna_integration/xgboost.py Show resolved Hide resolved
Copy link
Member

@y0z y0z left a comment

Choose a reason for hiding this comment

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

I think one change should be made.

Also, please add XGBoost to init.py as same as other integrations.

docs/source/reference/index.rst Outdated Show resolved Hide resolved
Copy link
Collaborator

@nabenabe0928 nabenabe0928 left a comment

Choose a reason for hiding this comment

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

Thank you for your PR, please address my comments:)

tests/test_xgboost.py Outdated Show resolved Hide resolved
docs/source/reference/index.rst Show resolved Hide resolved
docs/source/reference/index.rst Outdated Show resolved Hide resolved
@nabenabe0928
Copy link
Collaborator

nabenabe0928 commented Feb 8, 2024

@buruzaemon
Plus, could you please press Resolve conflicts and resolve the conflict?

Basically, the conflict is happening in pyproject.toml, but you can keep both torch and xgboost in pyproject.toml.

After addressing my comments and the conflict resolve, we can merge this PR!

@buruzaemon
Copy link
Contributor Author

... and finally, resolved that conflict in pyproject.toml. ptal

Copy link

codecov bot commented Feb 8, 2024

Codecov Report

Attention: 26 lines in your changes are missing coverage. Please review.

Comparison is base (661aa9c) 65.06% compared to head (3470d9b) 64.92%.

Files Patch % Lines
optuna_integration/xgboost.py 60.31% 25 Missing ⚠️
optuna_integration/__init__.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #65      +/-   ##
==========================================
- Coverage   65.06%   64.92%   -0.14%     
==========================================
  Files          25       26       +1     
  Lines        1869     1933      +64     
==========================================
+ Hits         1216     1255      +39     
- Misses        653      678      +25     

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

@y0z
Copy link
Member

y0z commented Feb 8, 2024

@buruzaemon
Copy link
Contributor Author

@buruzaemon

Could you please apply isort? https://github.com/optuna/optuna-integration/actions/runs/7824603273/job/21347417916?pr=65

sure, i am on that now...

Copy link
Collaborator

@nabenabe0928 nabenabe0928 left a comment

Choose a reason for hiding this comment

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

@buruzaemon
Here are the changes you should make by isort.

Comment on lines 1 to 7
import numpy as np
import pytest

import optuna
from optuna_integration._imports import try_import
from optuna_integration.xgboost import XGBoostPruningCallback
from optuna.testing.pruners import DeterministicPruner
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
import numpy as np
import pytest
import optuna
from optuna_integration._imports import try_import
from optuna_integration.xgboost import XGBoostPruningCallback
from optuna.testing.pruners import DeterministicPruner
import numpy as np
import optuna
from optuna.testing.pruners import DeterministicPruner
import pytest
from optuna_integration._imports import try_import
from optuna_integration.xgboost import XGBoostPruningCallback

Copy link
Contributor Author

Choose a reason for hiding this comment

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

... and this is done!

Comment on lines 1 to 4
from typing import Any

import optuna
import optuna_integration
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
from typing import Any
import optuna
import optuna_integration
from typing import Any
import optuna
import optuna_integration

Copy link
Contributor Author

Choose a reason for hiding this comment

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

... and this change to the imports is done as well!

@nabenabe0928 nabenabe0928 removed their assignment Feb 8, 2024
Copy link
Collaborator

@nabenabe0928 nabenabe0928 left a comment

Choose a reason for hiding this comment

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

Thank you for the changes, LGTM!

@nabenabe0928 nabenabe0928 merged commit dec6d6c into optuna:main Feb 8, 2024
9 checks passed
@y0z y0z removed their assignment Feb 8, 2024
@nabenabe0928 nabenabe0928 added this to the v3.6.0 milestone Feb 8, 2024
@nabenabe0928 nabenabe0928 added the feature Change that does not break compatibility, but affects the public interfaces. label Feb 8, 2024
@Alnusjaponica Alnusjaponica mentioned this pull request Feb 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Change that does not break compatibility, but affects the public interfaces.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants