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

[MRG+1] Deprecate fetch_mldata #11466

Merged
merged 7 commits into from Aug 18, 2018

Conversation

5 participants
@jnothman
Member

jnothman commented Jul 10, 2018

Fixes #11317

I have changed the use of fetch_mldata in examples, but not yet in benchmarks under the assumption that those will be replaced with fetch_openml soon enough (even if after 0.20 release).

Waiting on the OpenML fetcher (#11419) to be completed.

@jnothman jnothman changed the title from Deprecate fetch_mldata to [MRG] Deprecate fetch_mldata Jul 10, 2018

@jnothman jnothman changed the title from [MRG] Deprecate fetch_mldata to [WIP] Deprecate fetch_mldata Jul 10, 2018

@jnothman jnothman changed the title from [WIP] Deprecate fetch_mldata to [MRG] Deprecate fetch_mldata Jul 10, 2018

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Jul 11, 2018

Member

It seems there are bugs in scipy.io.arff.loadarff in our earliest supported dependencies. Maybe this will need to wait for the OpenML fetcher (#9908, #11419)

Member

jnothman commented Jul 11, 2018

It seems there are bugs in scipy.io.arff.loadarff in our earliest supported dependencies. Maybe this will need to wait for the OpenML fetcher (#9908, #11419)

@jnothman jnothman changed the title from [MRG] Deprecate fetch_mldata to [WIP] Deprecate fetch_mldata Jul 11, 2018

@jnothman jnothman added this to the 0.20 milestone Jul 11, 2018

@qinhanmin2014

This comment has been minimized.

Show comment
Hide comment
@qinhanmin2014

qinhanmin2014 Jul 14, 2018

Member

I'm fine with the deprecation, but do we have ways to make the examples more friendly? (e.g., make use of scikit-learn/examples-data) I'm a bit uncomfortable about things like from sklearn.externals.joblib import Memory and the long functions which download&preprocess the data.

Member

qinhanmin2014 commented Jul 14, 2018

I'm fine with the deprecation, but do we have ways to make the examples more friendly? (e.g., make use of scikit-learn/examples-data) I'm a bit uncomfortable about things like from sklearn.externals.joblib import Memory and the long functions which download&preprocess the data.

@amueller

This comment has been minimized.

Show comment
Hide comment
@amueller

amueller Jul 14, 2018

Member

all the fetchers can be removed when we add the openml loader, right? If not, we should upload those datasets to openml.

Member

amueller commented Jul 14, 2018

all the fetchers can be removed when we add the openml loader, right? If not, we should upload those datasets to openml.

@FollowKenny

This comment has been minimized.

Show comment
Hide comment
@FollowKenny

FollowKenny Jul 16, 2018

Contributor

@jnothman Hi, do you need help on this right now ?

Contributor

FollowKenny commented Jul 16, 2018

@jnothman Hi, do you need help on this right now ?

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Jul 16, 2018

Member

No, thanks @FollowKenny. The plan is to get fetch_openml merged before release, I hope, and fix the issues here through that.

Member

jnothman commented Jul 16, 2018

No, thanks @FollowKenny. The plan is to get fetch_openml merged before release, I hope, and fix the issues here through that.

@amueller amueller added the Blocker label Jul 16, 2018

@jorisvandenbossche jorisvandenbossche added this to PRs tagged in scikit-learn 0.20 Jul 16, 2018

@amueller amueller moved this from PRs tagged to Blockers in scikit-learn 0.20 Jul 16, 2018

@jnothman jnothman changed the title from [WIP] Deprecate fetch_mldata to [MRG] Deprecate fetch_mldata Aug 15, 2018

@jnothman jnothman changed the title from [MRG] Deprecate fetch_mldata to [WIP] Deprecate fetch_mldata Aug 15, 2018

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Aug 15, 2018

Member

This isn't perfect, but it's mergeable after #11419

Member

jnothman commented Aug 15, 2018

This isn't perfect, but it's mergeable after #11419

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Aug 15, 2018

Member

We should also look into uploading Mauna Loa to openml

Member

jnothman commented Aug 15, 2018

We should also look into uploading Mauna Loa to openml

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Aug 15, 2018

Member

I think if this goes green, I'll be bold and merge #11419 and create a long series of follow-up issues.

Member

jnothman commented Aug 15, 2018

I think if this goes green, I'll be bold and merge #11419 and create a long series of follow-up issues.

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Aug 15, 2018

Member

This is now ready for review.

Member

jnothman commented Aug 15, 2018

This is now ready for review.

@jnothman jnothman changed the title from [WIP] Deprecate fetch_mldata to [MRG] Deprecate fetch_mldata Aug 15, 2018

Downloading datasets from the mldata.org repository
---------------------------------------------------

This comment has been minimized.

@rth

rth Aug 15, 2018

Member

Do you think it's better to remove this section altogether rather than say put a deprecation note at the begining?

@rth

rth Aug 15, 2018

Member

Do you think it's better to remove this section altogether rather than say put a deprecation note at the begining?

@@ -257,7 +257,6 @@ Loaders
datasets.fetch_kddcup99
datasets.fetch_lfw_pairs
datasets.fetch_lfw_people
datasets.fetch_mldata

This comment has been minimized.

@rth

rth Aug 15, 2018

Member

Same here, maybe still keep the doc even if it's deprecated? Who knows maybe the website will work again in september, and there is still code using this. Those users will no longer be able to find any documentation about this function in the stable docs... A doc with a large deprecation warning would avoid us some opened issues from confused users.

@rth

rth Aug 15, 2018

Member

Same here, maybe still keep the doc even if it's deprecated? Who knows maybe the website will work again in september, and there is still code using this. Those users will no longer be able to find any documentation about this function in the stable docs... A doc with a large deprecation warning would avoid us some opened issues from confused users.

data = fetch_mldata('mauna-loa-atmospheric-co2').data
X = data[:, [1]]
y = data[:, 0]
def load_mauna_loa_atmospheric_c02():

This comment has been minimized.

@rth

rth Aug 15, 2018

Member

We should open an issue that this should be uploaded to openml, as you suggested?

@rth

rth Aug 15, 2018

Member

We should open an issue that this should be uploaded to openml, as you suggested?

Show outdated Hide outdated sklearn/datasets/tests/test_mldata.py Outdated
Show outdated Hide outdated sklearn/datasets/tests/test_mldata.py Outdated
@@ -1513,6 +1512,7 @@ To be removed in 0.22
:template: deprecated_function.rst
covariance.graph_lasso
datasets.fetch_mldata

This comment has been minimized.

@jnothman

jnothman Aug 15, 2018

Member

This will generate the docs with the large deprecation warning.

@jnothman

jnothman Aug 15, 2018

Member

This will generate the docs with the large deprecation warning.

Show outdated Hide outdated sklearn/datasets/tests/test_mldata.py Outdated
@amueller

This comment has been minimized.

Show comment
Hide comment
@amueller

amueller Aug 16, 2018

Member

are these test failures from master?

Member

amueller commented Aug 16, 2018

are these test failures from master?

@amueller

This comment has been minimized.

Show comment
Hide comment
@amueller

amueller Aug 16, 2018

Member

is that the failure that's fixed in #11831?

Member

amueller commented Aug 16, 2018

is that the failure that's fixed in #11831?

@rth

This comment has been minimized.

Show comment
Hide comment
@rth

rth Aug 17, 2018

Member

mldata doctest is failing as far as I can see. We might want to skip it in pytest_collection_modifyitems under conftest.py ?

Member

rth commented Aug 17, 2018

mldata doctest is failing as far as I can see. We might want to skip it in pytest_collection_modifyitems under conftest.py ?

@qinhanmin2014

LGTM, +1 to skip the test (maybe # doctest: +SKIP?)
Maybe we should include it in the rc

Show outdated Hide outdated doc/whats_new/v0.20.rst Outdated
Show outdated Hide outdated examples/linear_model/plot_sgd_early_stopping.py Outdated
@rth

This comment has been minimized.

Show comment
Hide comment
@rth

rth Aug 17, 2018

Member

(maybe # doctest: +SKIP?)

Definitely, I'm spending too much time writing pytest fixtures ..

Member

rth commented Aug 17, 2018

(maybe # doctest: +SKIP?)

Definitely, I'm spending too much time writing pytest fixtures ..

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Aug 18, 2018

Member

I just deleted the example. If someone wants to remind me the right way to disable it...

Member

jnothman commented Aug 18, 2018

I just deleted the example. If someone wants to remind me the right way to disable it...

@qinhanmin2014

LGTM, I'm fine with removing the doctests. We should encourage users to turn to openml.
Appveyor failure irrelevant.

@qinhanmin2014 qinhanmin2014 changed the title from [MRG] Deprecate fetch_mldata to [MRG+1] Deprecate fetch_mldata Aug 18, 2018

@rth

This comment has been minimized.

Show comment
Hide comment
@rth

rth Aug 18, 2018

Member

Merging, thanks @jnothman !

Member

rth commented Aug 18, 2018

Merging, thanks @jnothman !

@rth rth merged commit fc56da5 into scikit-learn:master Aug 18, 2018

4 of 5 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
ci/circleci: deploy Your tests passed on CircleCI!
Details
ci/circleci: python2 Your tests passed on CircleCI!
Details
ci/circleci: python3 Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

scikit-learn 0.20 automation moved this from Blockers to Done Aug 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment