Skip to content

More systematic checks that an estimator was fit before using its parameters #267

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 9 commits into from
Dec 4, 2019
Merged

More systematic checks that an estimator was fit before using its parameters #267

merged 9 commits into from
Dec 4, 2019

Conversation

RobinVogel
Copy link
Contributor

Solves #255

terrytangyuan
terrytangyuan approved these changes Dec 4, 2019
View reviewed changes
Copy link
Member

@terrytangyuan terrytangyuan left a comment

Choose a reason for hiding this comment

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

LGTM thanks!

@terrytangyuan terrytangyuan merged commit 7af910f into scikit-learn-contrib:master Dec 4, 2019
@terrytangyuan
Copy link
Member

@RobinVogel It seems like this PR might have caused master branch to fail. Could you take a look? https://travis-ci.org/scikit-learn-contrib/metric-learn/builds/620639370

@RobinVogel
Copy link
Contributor Author

Looking into it. The tests fail only on python 3 with skggm.
I had to install lapack before I could install skggm with pip with conda install -c conda-forge lapack,
followed by pip install skggm. For now I can't replicate the error of the travis log locally.

@bellet
Copy link
Member

bellet commented Dec 5, 2019

It is strange because all tests passed on merge:
https://travis-ci.org/scikit-learn-contrib/metric-learn/builds/618537553?utm_source=github_status&utm_medium=notification

@bellet bellet mentioned this pull request Dec 5, 2019
Closed
bellet
bellet reviewed Dec 5, 2019
View reviewed changes
Copy link
Member

@bellet bellet left a comment

Choose a reason for hiding this comment

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

Sorry to review a bit late but I see a few minor issues. In addition to the comments below, there is a problem when calling score_pairs which raises an error of the form: (error: AttributeError: 'ITML' object has no attribute 'preprocessor_'). I think we need a check_is_fitted(self, ['preprocessor_', 'components_']) there as well

@bellet
Copy link
Member

bellet commented Dec 9, 2019

I have re-run Travis to make sure it was not a glitch - but the errors are still there

@RobinVogel RobinVogel mentioned this pull request Dec 10, 2019
Merged
@bellet
Copy link
Member

bellet commented Dec 13, 2019

Fixing the failing tests in #270
The reason the tests passed in the PR but failed on master after merging is because the new version of sklearn was released in between the two! Pretty big coincidence

bellet pushed a commit that referenced this pull request Jan 24, 2020
@RobinVogel @bellet
Corrects the forgotten bits of PR #267 (#269)
2380f51 
* maj

* maj

* corrected PR 267

* trailing whitespace

* test calibrate_threshold, test predict

* maj

* Checks estimator is fitted before set threshold

* correct failed tests with MockBadClassifier

* remove checks

* forgot one

* missed one check_is_fitted

* sklearn changed the assumptions behind check_is_fitted
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bellet bellet bellet left review comments

@terrytangyuan terrytangyuan terrytangyuan approved these changes

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
@RobinVogel @terrytangyuan @bellet