EHN Add parameter as_frame to fetch_covtype #17491

tianchuliang · 2020-06-06T18:28:07Z

Reference Issues/PRs

Reference: #10733

What does this implement/fix? Explain your changes.

This adds as_frame argument for covtype dataset loader.

Any other comments?

amueller · 2020-06-06T18:46:25Z

Thanks for the PR. Generally looks good, please check the linting error.

tianchuliang · 2020-06-06T18:48:50Z

@amueller yep, will resolve linter's complain :)

reshamas · 2020-06-06T22:57:16Z

#DataUmbrella sprint

adrinjalali

Thanks for the PR @tianchuliang

adrinjalali · 2020-06-08T09:41:46Z

sklearn/datasets/_covtype.py

@@ -80,6 +82,8 @@ def fetch_covtype(*, data_home=None, download_if_missing=True,
        If True, returns ``(data.data, data.target)`` instead of a Bunch
        object.

+    as_frame : boolean, default=False.
+        If True, returns ``pandas.DataFrame`` instead of a Bunch object


the result will still be a bunch object, it's the X and y which are a DataFrame instead of an ndarray.

the result is a bunch object, I added .frame attribute to the bunch object. The frame attribute is a combined dataframe of X and y, with properly specified columns. Please see discussion #10733 for reference, this is the design from that thread of discussion.

Yes, but here you say If True, returns ``pandas.DataFrame`` instead of a Bunch object which is not correct.

You can see the documentation and the implementation https://github.com/scikit-learn/scikit-learn/blob/master/sklearn/datasets/_base.py#L297 as an example

ok, got it. will adjust, thanks!

adrinjalali · 2020-06-08T09:42:36Z

sklearn/datasets/_covtype.py

@@ -145,4 +152,32 @@ def fetch_covtype(*, data_home=None, download_if_missing=True,
    if return_X_y:


the if as_frame part should be before this if.

adjusted accordingly, thanks for pointing it out.

adrinjalali · 2020-06-09T08:25:41Z

You may have not pushed your changes to this branch yet. Once you do I can give another review :)

tianchuliang · 2020-06-11T22:13:11Z

@adrinjalali yep, it's been a busy week this week; but I will look at the codecov issue this weekend and try to get it pass all checks.

amueller · 2020-06-15T17:04:15Z

the codecov error seems strange and unrelated, or I'm not looking at the right thing. Maybe don't worry about this one for now and just address the other comments.

tianchuliang · 2020-06-16T02:24:01Z

@amueller just pushed changes to address previous comments; the codecov error is about some of the if statements not being fully tested out. It's a bit tedious to add in all the tests, let me know if they needed to be done.

glemaitre · 2020-06-16T07:31:51Z

sklearn/datasets/_covtype.py

+        appropriate dtypes (numeric). The target is
+        a pandas DataFrame or Series depending on the number of target columns.
+        If `return_X_y` is True, then (`data`, `target`) will be pandas
+        DataFrames or Series as described below.
        .. versionadded:: 0.20


This version added was part of the previous parameter. You should add another versionadded for this parameter

.. versionadded:: 0.24

glemaitre · 2020-06-16T07:36:40Z

sklearn/datasets/_covtype.py

@@ -98,6 +106,9 @@ def fetch_covtype(*, data_home=None, download_if_missing=True,

    (data, target) : tuple if ``return_X_y`` is True

+    dataframe: :class: `pandas.DataFrame`


This should be added under DESCPR

glemaitre · 2020-06-16T07:38:18Z

sklearn/datasets/_covtype.py

@@ -98,6 +106,9 @@ def fetch_covtype(*, data_home=None, download_if_missing=True,

    (data, target) : tuple if ``return_X_y`` is True

+    dataframe: :class: `pandas.DataFrame`


Our guideline would say to write

frame : dataframe of shape (581012, 53)

glemaitre · 2020-06-16T07:38:54Z

sklearn/datasets/_covtype.py

@@ -98,6 +106,9 @@ def fetch_covtype(*, data_home=None, download_if_missing=True,

    (data, target) : tuple if ``return_X_y`` is True

+    dataframe: :class: `pandas.DataFrame`
+        Pandas dataframe


Suggested change

Pandas dataframe

Only present when `as_frame=True`. Contains `data` and `target`.

glemaitre · 2020-06-16T07:42:12Z

sklearn/datasets/_covtype.py

+        Column names reference:
+        https://archive.ics.uci.edu/ml/machine-learning-databases/covtype/covtype.info
+        """
+        feat_cols = ["Elevation",


Let's define those as constant on the top of the file.

rename feat_cols by FEATURE_NAMES and target_col by TARGET_NAMES

glemaitre · 2020-06-16T07:53:23Z

sklearn/datasets/_covtype.py

+        feat_cols += ['Soil_Type_'+str(i) for i in range(1, 41)]
+        target_col = ["Cover_Type"]
+
+        frame, X, y = _convert_data_dataframe("fetch_covtype", X, y,


call the function by passing only by keyword arguments it will be more explicit

glemaitre · 2020-06-16T07:55:17Z

sklearn/datasets/_covtype.py

@@ -142,7 +153,37 @@ def fetch_covtype(*, data_home=None, download_if_missing=True,
    with open(join(module_path, 'descr', 'covtype.rst')) as rst_file:
        fdescr = rst_file.read()

-    if return_X_y:
-        return X, y
+    if as_frame or return_X_y:


you can have something like

frame = None if as_frame: _convert_to_frame(...) if return_X_y: return data, target return Bunch(...)

glemaitre · 2020-06-16T08:01:31Z

sklearn/datasets/tests/test_covtype.py

+import pytest
 from sklearn.datasets.tests.test_common import check_return_X_y
 from functools import partial


Suggested change

import pytest

from sklearn.datasets.tests.test_common import check_return_X_y

from functools import partial

from functools import partial

import pytest

from sklearn.datasets.tests.test_common import check_return_X_y

glemaitre · 2020-06-16T08:03:41Z

sklearn/datasets/tests/test_covtype.py

+    pd = pytest.importorskip('pandas')
+    bunch = fetch_covtype_fxt(as_frame=True)
+    frame = bunch.frame
+    assert hasattr(bunch, frame) is True


Suggested change

assert hasattr(bunch, frame) is True

assert hasattr(bunch, frame)

glemaitre · 2020-06-16T08:04:21Z

sklearn/datasets/tests/test_covtype.py

+def test_fetch_asframe(fetch_covtype_fxt):
+    pd = pytest.importorskip('pandas')
+    bunch = fetch_covtype_fxt(as_frame=True)
+    frame = bunch.frame


The assert below will be useful because the test will already crash here if frame does not exist

adrinjalali · 2020-06-16T08:18:09Z

It's a bit tedious to add in all the tests, let me know if they needed to be done.

Writing the tests is in many cases more work than fixing the issue or implementing a feature itself; yet we need them to ensure the correctness and the quality of the code, and in many cases writing tests will uncover a few bugs which were not seen before. So yes, please add the tests.

glemaitre · 2020-06-16T12:47:20Z

sklearn/datasets/tests/test_covtype.py

+    frame = bunch.frame
+    assert hasattr(bunch, frame) is True
+    assert frame.shape == (581012, 55)
+    assert isinstance(bunch.frame, pd.DataFrame)


So the same here. It should be in the common test regarding the type checking. You can only check the shape of the data

ok, will get rid of the type checking here.

tianchuliang · 2020-06-19T17:00:51Z

@amueller @glemaitre hey guys, I am confused by the codecov/patch error; I have no idea what I am supposed to do because I can't interpret the diff <-> coverage change. As far as I can see, there are improved code coverages due to my diff. Please help, this codecov/patch error is quite irratating as it is not helping.

tianchuliang · 2020-06-19T17:03:20Z

@amueller @glemaitre As you can see, the two files where my code changes live show positive increased codecov. What's happening with this error?

thomasjpfan · 2020-06-19T19:03:57Z

The fetch_* functions are skipped on the CI with PRs thus the coverage would not measured for them. Only the build on travis will run the fetch_* tests. You can activate them by adding a [scipy-dev] to an empty commit message and push to this PR. (There may be errors that are unrelated to this PR because [scipy-dev] test on the development version of our dependencies.

thomasjpfan · 2020-06-19T21:02:43Z

Going over our travis config, it looks we do not upload coverage there either. This was done to not have false positive with PRs that never run the fetch_* tests.

Given that, it looks like it did pass the travis ci.

tianchuliang · 2020-06-20T14:13:44Z

@glemaitre @amueller @thomasjpfan is there a definitive answer as to what I should do with the codecov/patch issue? The amount of comments and rework on this PR is getting long IMO, I'd really appreciate it if there is a final set of clear check points, so we can close this?

thomasjpfan · 2020-06-20T14:35:18Z

is there a definitive answer as to what I should do with the codecov/patch issue? T

I think we ignore it for now.

The amount of comments and rework on this PR is getting long IMO,

This is the norm for non-trival PRs. Every merged PR increases the maintenance of the project, thus we do reviews and ask for code changes to keep the code quality high.

Thank you for your patience.

amueller

Thanks, looks good apart from some nitpicks!

amueller · 2020-06-20T15:16:25Z

sklearn/datasets/_covtype.py

@@ -80,7 +100,13 @@ def fetch_covtype(*, data_home=None, download_if_missing=True,
        If True, returns ``(data.data, data.target)`` instead of a Bunch
        object.

-        .. versionadded:: 0.20


I don't think this should be removed.

OK, I will add it back, thanks for pointing it out.

amueller · 2020-06-20T15:16:34Z

sklearn/datasets/_covtype.py

@@ -80,7 +100,13 @@ def fetch_covtype(*, data_home=None, download_if_missing=True,
        If True, returns ``(data.data, data.target)`` instead of a Bunch
        object.

-        .. versionadded:: 0.20
+    as_frame : bool, default=False


This is the part that's missing in the other PR.

Got it, thanks

amueller · 2020-06-20T15:16:54Z

sklearn/datasets/_covtype.py

        DESCR : str
            Description of the forest covertype dataset.

    (data, target) : tuple if ``return_X_y`` is True

-        .. versionadded:: 0.20


This shouldn't be changed, I think?

changing it back

amueller

Thanks, looks good!

thomasjpfan

LGTM

thomasjpfan · 2020-06-20T18:43:31Z

Please add an entry to the change log at doc/whats_new/v0.24.rst with tag |Enhancement|. Like the other entries there, please reference this pull request with :pr: and credit yourself (and other contributors if applicable) with :user:.

…ng/scikit-learn into 10733_fetch_covtype_return_df

thomasjpfan · 2020-06-21T02:20:08Z

doc/whats_new/v0.24.rst

@@ -53,6 +53,12 @@ Changelog
  unless data is sparse.
  :pr:`17396` by :user:`Jiaxiang <fujiaxiang>`.

+- |Enhancement| :func:`datasets.fetch_covtype` now now supports optional
+  argument `as_frame`; when it is set to true, the returned Bunch object's
+  `data` and `target` members are in pandas DataFrame format, and the Bunch


In this case, target is a pandas series.

I updated this, will merge when test pass

@thomasjpfan Thank you sir!

thomasjpfan · 2020-06-21T05:02:15Z

Merging the tests that are failing are also failing on master. I'll work on a fix tomorrow if it is not fixed by then.

Thank you for the PR @tianchuliang !

Co-authored-by: aliang <aliang@air> Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>

aliang added 2 commits June 6, 2020 14:12

auto flake8 stuff

322ea45

tests passed

c5f17eb

github-actions bot added the module:datasets label Jun 6, 2020

adjust lineendings

9ab6f5c

fixing flake8 linting issue

24df8fa

adrinjalali reviewed Jun 8, 2020

View reviewed changes

adjusted API

5a34ebf

flake8 error

04feec5

glemaitre changed the title ~~#10733 fetch covtype return df~~ EHN Add parameter as_frame to fetch_covtype Jun 16, 2020

glemaitre reviewed Jun 16, 2020

View reviewed changes

aliang added 2 commits June 19, 2020 11:56

Merge branch 'master' into 10733_fetch_covtype_return_df

2413b3b

trailing white spaces

f8ff415

[scipy-dev]

0134c73

amueller reviewed Jun 20, 2020

View reviewed changes

versionadded error fixes

7e06f8f

amueller approved these changes Jun 20, 2020

View reviewed changes

thomasjpfan added 4 commits June 20, 2020 12:32

ENH Update style and starting idx

8fc854e

BLD [scipy-dev]

6729f9a

DOC Use comment

7b77c03

DOC documents target and feature names

f9ee162

thomasjpfan approved these changes Jun 20, 2020

View reviewed changes

aliang added 2 commits June 20, 2020 22:09

added what's new

5ecc820

Merge branch '10733_fetch_covtype_return_df' of github.com:tianchulia…

75c5e8c

…ng/scikit-learn into 10733_fetch_covtype_return_df

thomasjpfan reviewed Jun 21, 2020

View reviewed changes

thomasjpfan added 3 commits June 20, 2020 22:26

DOC Update whats enw

335b4bf

TST Adds one more test

85718f3

Merge remote-tracking branch 'upstream/master' into pr/17491

93053f8

thomasjpfan merged commit 6ef4487 into scikit-learn:master Jun 21, 2020
6 of 7 checks passed

dsandeep0138 pushed a commit to dsandeep0138/scikit-learn that referenced this pull request Jun 21, 2020

EHN Add parameter as_frame to fetch_covtype (scikit-learn#17491)

a21b213

Co-authored-by: aliang <aliang@air> Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>

tianchuliang deleted the 10733_fetch_covtype_return_df branch June 21, 2020 18:31

alfaro96 mentioned this pull request Jun 23, 2020

Tests in "datasets" module are not skipped if pillow and pandas are not installed #17680

Closed

rubywerman pushed a commit to MLH-Fellowship/scikit-learn that referenced this pull request Jun 24, 2020

EHN Add parameter as_frame to fetch_covtype (scikit-learn#17491)

a3873e6

Co-authored-by: aliang <aliang@air> Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>

viclafargue pushed a commit to viclafargue/scikit-learn that referenced this pull request Jun 26, 2020

EHN Add parameter as_frame to fetch_covtype (scikit-learn#17491)

ee392ee

Co-authored-by: aliang <aliang@air> Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>

jayzed82 pushed a commit to jayzed82/scikit-learn that referenced this pull request Oct 22, 2020

EHN Add parameter as_frame to fetch_covtype (scikit-learn#17491)

78f86fb

Co-authored-by: aliang <aliang@air> Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>

		@@ -145,4 +152,32 @@ def fetch_covtype(*, data_home=None, download_if_missing=True,
		if return_X_y:

		@@ -98,6 +106,9 @@ def fetch_covtype(*, data_home=None, download_if_missing=True,

		(data, target) : tuple if ``return_X_y`` is True

		dataframe: :class: `pandas.DataFrame`

	Pandas dataframe
	Only present when `as_frame=True`. Contains `data` and `target`.

	assert hasattr(bunch, frame) is True
	assert hasattr(bunch, frame)

EHN Add parameter as_frame to fetch_covtype #17491

EHN Add parameter as_frame to fetch_covtype #17491

Conversation

tianchuliang commented Jun 6, 2020

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

amueller commented Jun 6, 2020

tianchuliang commented Jun 6, 2020

reshamas commented Jun 6, 2020

adrinjalali left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrinjalali commented Jun 9, 2020

tianchuliang commented Jun 11, 2020

amueller commented Jun 15, 2020

tianchuliang commented Jun 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrinjalali commented Jun 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tianchuliang commented Jun 19, 2020

tianchuliang commented Jun 19, 2020

thomasjpfan commented Jun 19, 2020

thomasjpfan commented Jun 19, 2020 • edited

tianchuliang commented Jun 20, 2020

thomasjpfan commented Jun 20, 2020

amueller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amueller left a comment

Choose a reason for hiding this comment

thomasjpfan left a comment

Choose a reason for hiding this comment

thomasjpfan commented Jun 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomasjpfan commented Jun 21, 2020

thomasjpfan commented Jun 19, 2020 •

edited