BUG Fixes fetch_kddcup99 for return_X_y and as_frame #19011
Conversation
There was a problem hiding this comment.
LGTM. I run the tests locally and it works. Thanks @thomasjpfan
There was a problem hiding this comment.
Codecov asserts that the the return_X_y == True case is not covered by the common test:
I suppose that this is because this check needs the network and is therefore skipped on the CI servers.
I tried to run the check locally and it passes:
% SKLEARN_SKIP_NETWORK_TESTS=0 pytest -vk fetch_kddcup99 sklearn/datasets/tests/test_common.py
==================================================================== test session starts ====================================================================
platform darwin -- Python 3.9.1, pytest-6.2.0, py-1.10.0, pluggy-0.13.1 -- /Users/ogrisel/miniforge3/envs/dev/bin/python
cachedir: .pytest_cache
rootdir: /Users/ogrisel/code/scikit-learn, configfile: setup.cfg
plugins: forked-1.2.0, xdist-2.1.0, timeout-1.4.2
collected 38 items / 35 deselected / 3 selected
sklearn/datasets/tests/test_common.py::test_common_check_return_X_y[fetch_kddcup99-fetch_kddcup99] PASSED [ 33%]
sklearn/datasets/tests/test_common.py::test_common_check_as_frame[fetch_kddcup99-fetch_kddcup99] PASSED [ 66%]
sklearn/datasets/tests/test_common.py::test_common_check_pandas_dependency[fetch_kddcup99-fetch_kddcup99] SKIPPED (This test requires pandas to n...) [100%]
================================================================== short test summary info ==================================================================
SKIPPED [1] sklearn/datasets/tests/test_common.py:43: This test requires pandas to not be installed
======================================================== 2 passed, 1 skipped, 35 deselected in 9.39s ========================================================
| assert isinstance(frame_X, pd.DataFrame) | ||
| else: | ||
| assert isinstance(frame_y, pd.Series) | ||
|
|
||
|
|
||
| def _skip_network_tests(): | ||
| return os.environ.get('SKLEARN_SKIP_NETWORK_TESTS', '1') == '1' |
There was a problem hiding this comment.
Shouldn't it be:
| return os.environ.get('SKLEARN_SKIP_NETWORK_TESTS', '1') == '1' | |
| return os.environ.get('SKLEARN_SKIP_NETWORK_TESTS', '0') == '1' |
by default?
There was a problem hiding this comment.
No strong opinion, but I was surprise to get those checks skipped by default on my local setup.
There was a problem hiding this comment.
I think we have no network as default to make it easier for repo maintainers such as debian. (One less configuration to set)
On a side note, it would be nice to have a [ci network] to enable this test on the CI for PRs.
There was a problem hiding this comment.
No strong opinion, but I was surprise to get those checks skipped by default on my local setup.
I got a little surprised at first as well :)
There was a problem hiding this comment.
+1 for [ci network]. In another PR.
|
To be backported to 0.24.X |
Reference Issues/PRs
Fixes #19010
What does this implement/fix? Explain your changes.
Adds test for
return_X_y=Trueandas_frame=Trueinto the common tests.