Skip to content

ck estimator is classifier & num_classes>=2 in score.py #12221

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

Closed
wants to merge 1 commit into from
Closed

ck estimator is classifier & num_classes>=2 in score.py #12221

wants to merge 1 commit into from

Conversation

AMDonati
Copy link

@AMDonati AMDonati commented Sep 29, 2018

Reference Issues/PRs

What does this implement/fix? Explain your changes.

We are fixing this issue: #7598
We added a test in the scorer.py file that raises a ValueError if the user is either trying to use a non classifier model for a classification problem, or is using a dataset with only one class.

Any other comments?

@reshamas

@@ -183,7 +183,12 @@ def __call__(self, clf, X, y, sample_weight=None):
y_pred = clf.predict_proba(X)

if y_type == "binary":
y_pred = y_pred[:, 1]
if y_pred.shape[1]==2:
Copy link
Member

Choose a reason for hiding this comment

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

Can you please run flake8 on this file? There are some formatting issues.

@amueller
Copy link
Member

Looks good in general, but you need to add a regression test (you could use the GMM one or just a classification one with a single class maybe)

@AMDonati
Copy link
Author

AMDonati commented Sep 30, 2018 via email

@reshamas
Copy link
Member

@AMDonati I am happy to get on a zoom or google hangouts meeting so we can run the checks. Let me know what works for you. My email: reshama@wimlds.org

@reshamas
Copy link
Member

reshamas commented Nov 1, 2018

@AMDonati can you close this PR?
Reference: 12486

@AMDonati
Copy link
Author

AMDonati commented Nov 1, 2018

@reshamas , I will try to close it tomorrow.

@sergulaydore
Copy link
Contributor

Hello @AMDonati ,

Thank you for participating in the WiMLDS/scikit sprint. We would love to merge all the PRs that were submitted. It would be great if you could follow up on the work that you started! For the PR you submitted, would you please update and re-submit? Please include #wimlds in your PR conversation.

Any questions:

  • see workflow for reference
  • ask on this PR conversation or the issue tracker
  • ask on wimlds gitter with a reference to this PR

cc: @reshamas

@AMDonati
Copy link
Author

AMDonati commented Nov 14, 2018 via email

jnothman pushed a commit that referenced this pull request Nov 14, 2018
jnothman pushed a commit to jnothman/scikit-learn that referenced this pull request Nov 20, 2018
xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019
koenvandevelde pushed a commit to koenvandevelde/scikit-learn that referenced this pull request Jul 12, 2019
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.

4 participants