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

BUG: for several datasets, ``download_if_missing`` keyword was ignored. #7944

Merged
merged 1 commit into from Nov 29, 2016

Conversation

Projects
None yet
5 participants
@rgommers
Contributor

rgommers commented Nov 27, 2016

Do what is promised in the docstring (raise an IOError), if download_if_missing=False and the dataset wasn't previously downloaded.

@agramfort

This comment has been minimized.

Show comment
Hide comment
@agramfort

agramfort Nov 27, 2016

Member

LGTM but can you look why CIs are not happy?

Member

agramfort commented Nov 27, 2016

LGTM but can you look why CIs are not happy?

@rgommers

This comment has been minimized.

Show comment
Hide comment
@rgommers

rgommers Nov 28, 2016

Contributor

LGTM but can you look why CIs are not happy?

A combination of me being asleep and running the tests for the wrong version of sklearn, and 2 tests being a bit odd with checking for a particular error code. Fixed now.

Contributor

rgommers commented Nov 28, 2016

LGTM but can you look why CIs are not happy?

A combination of me being asleep and running the tests for the wrong version of sklearn, and 2 tests being a bit odd with checking for a particular error code. Fixed now.

if e.errno == errno.ENOENT:
raise SkipTest("Covertype dataset can not be loaded.")
except IOError:
raise SkipTest("Covertype dataset can not be loaded.")

This comment has been minimized.

@tguillemot

tguillemot Nov 28, 2016

Contributor

@agramfort I know it's an old code but why use a SkipTest rather than an assert_raise_message ?

@tguillemot

tguillemot Nov 28, 2016

Contributor

@agramfort I know it's an old code but why use a SkipTest rather than an assert_raise_message ?

This comment has been minimized.

@amueller

amueller Nov 28, 2016

Member

We don't know whether the call will succeed, right?

@amueller

amueller Nov 28, 2016

Member

We don't know whether the call will succeed, right?

This comment has been minimized.

@tguillemot

tguillemot Nov 28, 2016

Contributor

Of course. Thanks for the clarification and sorry for the misunderstanding @amueller.

@tguillemot

tguillemot Nov 28, 2016

Contributor

Of course. Thanks for the clarification and sorry for the misunderstanding @amueller.

@tguillemot

This comment has been minimized.

Show comment
Hide comment
@tguillemot

tguillemot Nov 28, 2016

Contributor

LGTM despite the SkipTest :)

Contributor

tguillemot commented Nov 28, 2016

LGTM despite the SkipTest :)

@amueller

This comment has been minimized.

Show comment
Hide comment
@amueller

amueller Nov 28, 2016

Member

regression test would be checking that we get an IOError if the file does not exist, right? Might be a bit overkill? Otherwise LGTM.

Member

amueller commented Nov 28, 2016

regression test would be checking that we get an IOError if the file does not exist, right? Might be a bit overkill? Otherwise LGTM.

@TomDLT

This comment has been minimized.

Show comment
Hide comment
@TomDLT

TomDLT Nov 29, 2016

Member

LGTM

Member

TomDLT commented Nov 29, 2016

LGTM

@amueller amueller merged commit 63583fe into scikit-learn:master Nov 29, 2016

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@amueller

This comment has been minimized.

Show comment
Hide comment
@amueller

amueller Nov 29, 2016

Member

uh that needs a whatsnew. Anyone wanna have a go?

Member

amueller commented Nov 29, 2016

uh that needs a whatsnew. Anyone wanna have a go?

@rgommers rgommers deleted the rgommers:dowload-if-missing branch Nov 29, 2016

@rgommers

This comment has been minimized.

Show comment
Hide comment
@rgommers

rgommers Nov 29, 2016

Contributor

You mean a mention under "bug fixes" in doc/whats_new.rst? I can send a new PR for that.

Contributor

rgommers commented Nov 29, 2016

You mean a mention under "bug fixes" in doc/whats_new.rst? I can send a new PR for that.

@amueller

This comment has been minimized.

Show comment
Hide comment
@amueller

amueller Nov 29, 2016

Member

indeed. Thanks :)

Member

amueller commented Nov 29, 2016

indeed. Thanks :)

adis300 added a commit to adis300/scikit-learn that referenced this pull request Dec 13, 2016

sergeyf added a commit to sergeyf/scikit-learn that referenced this pull request Feb 28, 2017

Sundrique added a commit to Sundrique/scikit-learn that referenced this pull request Jun 14, 2017

NelleV added a commit to NelleV/scikit-learn that referenced this pull request Aug 11, 2017

paulha added a commit to paulha/scikit-learn that referenced this pull request Aug 19, 2017

maskani-moh added a commit to maskani-moh/scikit-learn that referenced this pull request Nov 15, 2017

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