Skip to content
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] Tests refactoring #242

Merged
merged 14 commits into from Mar 19, 2017
Merged

[MRG] Tests refactoring #242

merged 14 commits into from Mar 19, 2017

Conversation

@glemaitre
Copy link
Member

@glemaitre glemaitre commented Mar 13, 2017

Reference Issue

Fixes #238

What does this implement/fix? Explain your changes.

TODO:

  • Remove useless doctring from the tests;
  • Remove useless comments;
  • Create a test_common.py for the different samplers:
    • check_estimator;
    • Is BinarySampler or MulticlassSampler;
    • test ratio;
    • test single class;
    • test check_fitting -> we need to bring check_is_fit from sklearn;
    • test sampling wrong X.

Any other comments?

@pep8speaks
Copy link

@pep8speaks pep8speaks commented Mar 13, 2017

Hello @glemaitre! Thanks for updating the PR.

Comment last updated on March 17, 2017 at 20:16 Hours UTC
@codecov
Copy link

@codecov codecov bot commented Mar 13, 2017

Codecov Report

Merging #242 into master will decrease coverage by 0.67%.
The diff coverage is 96.13%.

@@            Coverage Diff             @@
##           master     #242      +/-   ##
==========================================
- Coverage   98.95%   98.27%   -0.68%     
==========================================
  Files          50       58       +8     
  Lines        4007     3429     -578     
==========================================
- Hits         3965     3370     -595     
- Misses         42       59      +17
Impacted Files Coverage Δ
imblearn/under_sampling/tests/test_allknn.py 100% <ø> (ø) ⬆️
imblearn/__init__.py 100% <ø> (ø) ⬆️
...earn/under_sampling/condensed_nearest_neighbour.py 100% <100%> (ø) ⬆️
imblearn/over_sampling/smote.py 93.6% <100%> (-0.43%) ⬇️
imblearn/exceptions.py 100% <100%> (ø)
...n/under_sampling/tests/test_one_sided_selection.py 100% <100%> (ø) ⬆️
imblearn/under_sampling/tests/test_nearmiss.py 100% <100%> (ø)
...sampling/tests/test_neighbourhood_cleaning_rule.py 100% <100%> (ø) ⬆️
imblearn/ensemble/tests/test_easy_ensemble.py 100% <100%> (ø) ⬆️
imblearn/under_sampling/nearmiss.py 97.64% <100%> (+2.04%) ⬆️
... and 45 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 768def7...590d600. Read the comment docs.

@glemaitre glemaitre changed the title [WIP] Tests refactoring [MRG] Tests refactoring Mar 17, 2017
@glemaitre glemaitre changed the title [MRG] Tests refactoring [WIP] Tests refactoring Mar 17, 2017
@glemaitre glemaitre force-pushed the glemaitre:fix_tests branch from fe97485 to 271d4cf Mar 17, 2017
@glemaitre glemaitre changed the title [WIP] Tests refactoring [MRG] Tests refactoring Mar 17, 2017
@glemaitre
Copy link
Member Author

@glemaitre glemaitre commented Mar 17, 2017

@chkoar You were right the refactoring was worth when checking at the number of lines suppressed.
I manage also to solve some issue on the path that I did not figure out earlier. Now we through an error when there is only one class which make much more sense.

It is good for merging

@glemaitre
Copy link
Member Author

@glemaitre glemaitre commented Mar 17, 2017

You can check codecov but I don't think that the difference of coverage is a problem. This is difficult to cover the tools used for testing ;)
They are coming from sklearn so there so case that we are not using for the moment. I am not sure this is a good idea to remove those parts

@glemaitre glemaitre changed the title [MRG] Tests refactoring [WIP] Tests refactoring Mar 17, 2017
@glemaitre glemaitre changed the title [WIP] Tests refactoring [MRG] Tests refactoring Mar 17, 2017
@glemaitre
Copy link
Member Author

@glemaitre glemaitre commented Mar 17, 2017

@chkoar I think that this is good for merging this time

@chkoar chkoar merged commit ee7fcb3 into scikit-learn-contrib:master Mar 19, 2017
4 of 6 checks passed
4 of 6 checks passed
@codecov
codecov/patch 96.13% of diff hit (target 98.95%)
Details
@codecov
codecov/project 98.27% (-0.68%) compared to 768def7
Details
ci/circleci Your tests passed on CircleCI!
Details
code-quality/landscape Code quality decreased by -0.82%
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@chkoar
Copy link
Member

@chkoar chkoar commented Mar 19, 2017

Thanks

christophe-rannou pushed a commit to christophe-rannou/imbalanced-learn that referenced this pull request Apr 3, 2017
Remove useless docstring in tests
Add utils and common test to check estimator
Add test for meta-classifiers
Factorize tests
Add SkipTest from scikit-learn
Add missing tests
Remove useless tests
glemaitre added a commit to glemaitre/imbalanced-learn that referenced this pull request Jun 15, 2017
Remove useless docstring in tests
Add utils and common test to check estimator
Add test for meta-classifiers
Factorize tests
Add SkipTest from scikit-learn
Add missing tests
Remove useless tests
glemaitre added a commit to glemaitre/imbalanced-learn that referenced this pull request Jun 15, 2017
Remove useless docstring in tests
Add utils and common test to check estimator
Add test for meta-classifiers
Factorize tests
Add SkipTest from scikit-learn
Add missing tests
Remove useless tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants