TST make sklearn integration test compatible with 0.24 #3533
Conversation
glemaitre
requested review from
chivee,
henry0312,
StrikerRUS and
wxchan
as code owners
|
|
| for check in _yield_all_checks(name, estimator): | ||
| check_name = check.func.__name__ if hasattr(check, 'func') else check.__name__ | ||
| if check_name == 'check_estimators_nan_inf': | ||
| continue # skip test because LightGBM deals with nan |
There was a problem hiding this comment.
This should be handled by the tag allow_nan.
| check_name = check.func.__name__ if hasattr(check, 'func') else check.__name__ | ||
| if check_name == 'check_estimators_nan_inf': | ||
| continue # skip test because LightGBM deals with nan | ||
| elif check_name == "check_no_attributes_set_in_init": |
There was a problem hiding this comment.
This should be handle by the xfail that I added
| @pytest.mark.skipif( | ||
| sk_version < "0.23.0", reason="scikit-learn version is less than 0.23" | ||
| ) | ||
| @parametrize_with_checks(list(_tested_estimators())) |
There was a problem hiding this comment.
Only the parametrize_with_checks was using _xfail in 0.23. It has the advantage to show all the test from _yield as with parametrize of pytest.
There was a problem hiding this comment.
However, it needs to be a pure function not embedded within a class
There was a problem hiding this comment.
@glemaitre Thank you very much for updating LightGBM to follow the most recent scikit-learn API changes! Really appreciate it!
Especially thanks a lot for preserving tests for old versions, because we try to target the most wide range of versions.
Generally LGTM! Just please consider checking a few my minor comments below.
| xfail_checks = estimator._get_tags()["_xfail_checks"] | ||
| check_name = check.__name__ if hasattr(check, "__name__") else check.func.__name__ | ||
| if xfail_checks and check_name in xfail_checks: |
There was a problem hiding this comment.
With get() it looks a bit safer.
| xfail_checks = estimator._get_tags()["_xfail_checks"] | |
| check_name = check.__name__ if hasattr(check, "__name__") else check.func.__name__ | |
| if xfail_checks and check_name in xfail_checks: | |
| xfail_checks = estimator._get_tags().get("_xfail_checks", {}) | |
| check_name = check.__name__ if hasattr(check, "__name__") else check.func.__name__ | |
| if check_name in xfail_checks: |
There was a problem hiding this comment.
"_xfail_checks" is always defined because it is a default tag. At least, if there is a breaking change in the future, one could detect it with the failure. WDYT?
There was a problem hiding this comment.
Ah, so _xfail_checks is defined (with empty dict?) in BaseEstimator, if I got it correctly? Then it makes sense!
Then another question. Why do we need if xfail_checks and ... guard?
UPD: Sorry, never mind! Just found the answer: https://github.com/scikit-learn/scikit-learn/blob/1864ed442e9df17007996080946fe86789c63b8f/sklearn/base.py#L35. Default value is False, not {} as I thought.
|
Fixes #2947. |
|
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. |
Modify the integration test for scikit-learn. Some deprecations have been raised in 0.23 and the upcoming release of 0.24 will break this test.