Skip to content

[MRG+1] Hide train_test_split from nose test discovery. #10223

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Dec 6, 2017
Merged

[MRG+1] Hide train_test_split from nose test discovery. #10223

merged 2 commits into from
Dec 6, 2017

Conversation

jbearer
Copy link
Contributor

@jbearer jbearer commented Nov 29, 2017

This was removed when scikit-learn switched from nose to pytest.
However, other projects that still use nose may import this function
into their test suites. For example, scikit-learn-garden does this.

@jbearer
Hide train_test_split from nose test discovery.
8f43ca7 
This was removed when scikit-learn switched from nose to pytest.
However, other projects that still use nose may import this function
into their test suites. For example, scikit-learn-garden does this.
@amueller
Copy link
Member

flake8 fails.
Other than that I'm happy to leave that in for now.

@amueller
Copy link
Member

though then the question is when would it be safe to remove? This could also be done on the scikit-learn-garden side, right?

@jnothman
Copy link
Member

Given the abounding popularity of train_test_split, I think we can play friendly and not break people's test suites for the sake of cleanliness. Let's accept this after the flake8 fix, and with a comment stating what it's for.

jnothman
jnothman reviewed Dec 3, 2017
View reviewed changes
Copy link
Member

@jnothman jnothman left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Still not certain what consensus is though

@jnothman jnothman changed the title [MRG] Hide train_test_split from nose test discovery. [MRG+1] Hide train_test_split from nose test discovery. Dec 3, 2017
@jbearer
Copy link
Contributor Author

jbearer commented Dec 6, 2017

FWIW I think it makes sense to merge this. train_test_split isn't a test, so it should never be treated as one. And it makes sense to fix it here at the root of the problem rather than building this fix into every test suite that uses it.

That said, I don't know how big of a problem this is. If it's only scikit garden that has this issue, it could probably be fixed there.

@jnothman
Copy link
Member

jnothman commented Dec 6, 2017

Ending the agony...

@jnothman jnothman merged commit ea6ea81 into scikit-learn:master Dec 6, 2017
jwjohnson314 pushed a commit to jwjohnson314/scikit-learn that referenced this pull request Dec 18, 2017
@jbearer
MAINT Hide train_test_split from nose test discovery. (scikit-learn#1…
01a2c6b 
…0223)
@trevorstephens trevorstephens mentioned this pull request May 26, 2019
Closed
@jnothman jnothman mentioned this pull request May 26, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jnothman jnothman jnothman left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jbearer @amueller @jnothman