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.

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

Fix bugs in robust module and improve test coverage #78

Merged
merged 50 commits into from
Apr 8, 2021
Merged

Fix bugs in robust module and improve test coverage #78

merged 50 commits into from
Apr 8, 2021

Conversation

TimotheeMathieu
Copy link
Contributor

@TimotheeMathieu TimotheeMathieu commented Nov 17, 2020 •
edited
Loading

In PR #76 I succeeded in making examples faster but as a consequence some of the examples do not work anymore.
It allowed me to see some bugs in the robust module and this PR is to resolve them and try to improve the test in order not to encounter this situation again.

TODO:

  • add test for the weights_ parameter
  • add test to compare to already existing robust algo in regression in sklearn (TheilSen, Ransac)
  • add tests
  • fix examples

If anyone has a test suggestion, please share in comment.

TimotheeMathieu and others added 29 commits October 4, 2020 14:45
Creation of one class for each task classification, regression and cl…
7aa691f 
…ustering. Update doc, test, examples. Fix typo
Merge branch 'master' of https://github.com/scikit-learn-contrib/scik…
6144c84 
…it-learn-extra into robust
Update plot clustering
013a542
Update test_common
4793597
Fix test and corss_val
b813b75
fix doc function RobustWeightedEstimators
b945652
fix _RobustWeightedEstimator doc and RobustWeightedKMeans doc
a7c6f85
Black reformatted
a310116
Add Huber loss
2904da4
@rth
Merge remote-tracking branch 'upstream/master' into robust
4c28a92
Merge remote-tracking branch 'upstream/master' into robust
7736a54
Merge branch 'robust' of https://github.com/TimotheeMathieu/scikit-le…
875fa3c 
…arn-extra into robust
reformat black
20c6dea
Changes suggested by @rth in PR
147373f
Add stop criterion and change for faster examples
9e9cec9
resolve conflicts
bb9a46e
Subsample clustering
5d4e186
fix clustering example
27299eb
cython kmeans loss
5073e44
fix black
75374be
type conversion cython
5c7eb3e
fix setup.py windows
c2a2ae4
fix black
3a8f26a
Add losses and fix docstrings
df0cc9b
black and clean print
9fd1406
fix mistake in code
04c65fd
Fix diverse small problems. Better test coverage
f5decde
black
38e545c
Merge remote-tracking branch 'upstream/master' into robust
8f032f5
@TimotheeMathieu TimotheeMathieu added the bug Something isn't working label Nov 17, 2020
@TimotheeMathieu
Copy link
Contributor Author

TimotheeMathieu commented Nov 23, 2020 •
edited
Loading

@rth the PR is ready for review, thanks in advance. For now I removed the real data classification example as I didn't find a good example to replace the last one that had some problems.

rth
rth reviewed Nov 26, 2020
View reviewed changes
Copy link
Contributor

@rth rth left a comment

Choose a reason for hiding this comment

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

Thanks @TimotheeMathieu ! Generally looks good to me aside for the two following comments.

sklearn_extra/robust/robust_weighted_estimator.py Show resolved Hide resolved
sklearn_extra/robust/robust_weighted_estimator.py Outdated Show resolved Hide resolved
@TimotheeMathieu TimotheeMathieu mentioned this pull request Nov 26, 2020
Merged
@TimotheeMathieu
Copy link
Contributor Author

TimotheeMathieu commented Nov 26, 2020 •
edited
Loading

I fixed the test for kmedoids here mentioned in PR #73

EDIT : I removed the test in order to make a separate PR.

@TimotheeMathieu TimotheeMathieu changed the title Fix bugs in robust module and improve test coverage. Fix bugs in robust module and improve test coverage & fix test KMedoids. Nov 27, 2020
@TimotheeMathieu
Copy link
Contributor Author

@rth I don't understand the error in azure pipeline for python 3.9.
I can't reproduce it in a virtual env so I guess it is a problem with the azure pipeline.

@adrinjalali
Copy link
Member

The azure-pipelines.yaml needs fixing. The latest scipy release supports python3.9 already, shouldn't be fetching the nightly release. Not sure what's making it fetch from the nightly repo.

The scikit-learn pre-release also supports py3.9 already.

@TimotheeMathieu TimotheeMathieu changed the title Fix bugs in robust module and improve test coverage & fix test KMedoids. Fix bugs in robust module and improve test coverage Dec 21, 2020
@rth rth mentioned this pull request Dec 23, 2020
Merged
@TimotheeMathieu TimotheeMathieu requested a review from rth March 1, 2021 08:46
@TimotheeMathieu TimotheeMathieu mentioned this pull request Mar 5, 2021
Merged
rth
rth reviewed Apr 7, 2021
View reviewed changes
Copy link
Contributor

@rth rth left a comment

Choose a reason for hiding this comment

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

Sorry I won't be able to do a detailed review, but generally looks good to me. Thanks!

Feel free to merge once CI is green.

sklearn_extra/robust/mean_estimators.py Outdated Show resolved Hide resolved
@rth rth mentioned this pull request Apr 7, 2021
Closed
3 tasks
@TimotheeMathieu TimotheeMathieu merged commit 5b45d64 into scikit-learn-contrib:master Apr 8, 2021
@TimotheeMathieu TimotheeMathieu mentioned this pull request Apr 11, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rth rth rth 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
@TimotheeMathieu @rth @adrinjalali