Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

[MRG] Better error messages for bad contamination in outlier detectors #18780

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

Merged
merged 3 commits into from
Feb 12, 2021

Conversation

albertcthomas
Copy link
Contributor

Wrong contamination values for EllipticEnvelope and IsolationForest were not returning an appropriate ValueError as is the case for LocalOutlierFactor.

I also improved the common tests to catch this.

Copy link
Member

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

Not a specialist but the suggested change sounds reasonable. Just a tiny suggestion for the test:

sklearn/utils/estimator_checks.py Outdated Show resolved Hide resolved
Base automatically changed from master to main January 22, 2021 10:53
rth and others added 2 commits February 12, 2021 10:13
Copy link
Member

@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.

Not a specialist but the suggested change sounds reasonable.

Same for me. Thanks @albertcthomas !

@rth
Copy link
Member

rth commented Feb 12, 2021

The CircleCI lint failed with,

fatal: Couldn't find remote ref master
error: pathspec 'pr/18780/merge' did not match any file(s) known to git
Could not fetch merge commit.
There may be conflicts in merging PR #18780 with master.

but I just merged master, so I'm going to ignore it and hope it will be OK once merged. The Azure pipeline lint passes.

@rth rth merged commit e6e543b into scikit-learn:main Feb 12, 2021
@albertcthomas
Copy link
Contributor Author

Thanks for finishing this @rth and for the review @ogrisel

@glemaitre glemaitre mentioned this pull request Apr 22, 2021
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.