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

[tests][python][scikit-learn] New unit tests and maintenance #3253

Merged
merged 9 commits into from
Jul 30, 2020
Merged

[tests][python][scikit-learn] New unit tests and maintenance #3253

merged 9 commits into from
Jul 30, 2020

Conversation

a-wozniakowski
Copy link
Contributor

This pull request:

  • Includes new multioutput tests for independent subtasks and chaining.
  • Includes a RandomizedSearchCV test.
  • Updates dataset parameters to eliminate the FutureWarning in accordance with PEP 3102.
  • Updates the GridSearchCV test such that the refit best classifier is scored on the test data-- this is also done in the RandomizedSearchCV test.

* Includes multioutput tests
* Includes RandomizedSearchCV test
* Updates dataset parameters to eliminate FutureWarning
Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

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

@a-wozniakowski Thank you very much for fixing the FutureWarning and adding more tests! Given that scikit-learn public API changes in a non-backward compatible way (#2628) quite often and passing their check_estimator test doesn't mean that we are fully compatible with the most recent API (scikit-learn/scikit-learn#15392 (comment)), I believe that having more tests for their popular tools is very important!

Please check some minor comment below:

tests/python_package_test/test_sklearn.py Outdated Show resolved Hide resolved
tests/python_package_test/test_sklearn.py Outdated Show resolved Hide resolved
tests/python_package_test/test_sklearn.py Outdated Show resolved Hide resolved
tests/python_package_test/test_sklearn.py Outdated Show resolved Hide resolved
tests/python_package_test/test_sklearn.py Outdated Show resolved Hide resolved
tests/python_package_test/test_sklearn.py Outdated Show resolved Hide resolved
tests/python_package_test/test_sklearn.py Show resolved Hide resolved
tests/python_package_test/test_sklearn.py Outdated Show resolved Hide resolved
tests/python_package_test/test_sklearn.py Show resolved Hide resolved
tests/python_package_test/test_sklearn.py Outdated Show resolved Hide resolved
@StrikerRUS StrikerRUS changed the title [python][scikit-learn] New unit tests and maintenance [tests][python][scikit-learn] New unit tests and maintenance Jul 28, 2020
* Also updates validation split in grid and random search
@a-wozniakowski
Copy link
Contributor Author

@StrikerRUS thanks for your feedback! I implemented the requested changes, and I wrote the grid/random searches such that there is an explicit train/validation/test split.

Also, scikit-learn supports **fit_params in MultiOutputClassifier and MultiOutputRegressor; see issue #15953. This seems partial progress towards issue #524, which is contained in the feature request hub #2302.

Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

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

Thanks a lot for addressing comments! Please consider two check simplifications.

tests/python_package_test/test_sklearn.py Outdated Show resolved Hide resolved
tests/python_package_test/test_sklearn.py Outdated Show resolved Hide resolved
tests/python_package_test/test_sklearn.py Outdated Show resolved Hide resolved
tests/python_package_test/test_sklearn.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@StrikerRUS StrikerRUS 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 very much @a-wozniakowski !

@StrikerRUS StrikerRUS merged commit fed5752 into microsoft:master Jul 30, 2020
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants