Skip to content

[MRG] FIX Revert the addition of ndcg_score and dcg_score #9932

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 4 commits into from
Oct 16, 2017

Conversation

jnothman
Copy link
Member

This is an alternative to trying to fix up an unusable, undocumented implementation...

Closes #9921
Closes #9928
Closes #9929
Closes #9930
Closes #9931

@jnothman jnothman added this to the 0.19.1 milestone Oct 16, 2017
@jnothman
Copy link
Member Author

jnothman commented Oct 16, 2017

My only qualm with this is that dcg_score might actually work. But who'd know, as it's not tested nor correct behaviour documented.

@jeromedockes
Copy link
Contributor

My only qualm with this is that dcg_score might actually work. But who'd know, as it's not tested nor correct behaviour documented.

I think dcg_score is correct, although the docstring isn't. also, if y_true is binary, I don't see what

y_true = 2 ** y_true - 1 

achieves

@@ -20,3 +20,5224 @@ Previous Releases
Version 0.14 <whats_new/v0.14.rst>
Version 0.13 <whats_new/v0.13.rst>
Older Versions <whats_new/older_versions.rst>

Copy link
Member

Choose a reason for hiding this comment

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

No need of the old whats_new

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed!

@jnothman jnothman changed the title FIX Revert the addition of ndcg_score and dcg_score [MRG] FIX Revert the addition of ndcg_score and dcg_score Oct 16, 2017
@jnothman
Copy link
Member Author

@TomDLT, opinion on the reversion?

Copy link
Member

@qinhanmin2014 qinhanmin2014 left a comment

Choose a reason for hiding this comment

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

personally +1 for the PR :)

from ..utils.multiclass import type_of_target
from ..utils.extmath import stable_cumsum
from ..utils.sparsefuncs import count_nonzero
from ..exceptions import UndefinedMetricWarning
from ..preprocessing import label_binarize
Copy link
Member

Choose a reason for hiding this comment

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

Seems that we need label_binarize here, it's now used in roc_auc_score for supporting binary y_true other than {0, 1} / {-1, 1}.

@TomDLT
Copy link
Member

TomDLT commented Oct 16, 2017

I agree it's simpler to revert it.

We probably need to document the reversion, adding for example a descriptive error for users that will try to import these metrics.

@jnothman
Copy link
Member Author

jnothman commented Oct 16, 2017 via email

@qinhanmin2014
Copy link
Member

if we do not provide a message for the revert, do we need to remove the what's new entry in 0.19?

@jnothman
Copy link
Member Author

jnothman commented Oct 16, 2017 via email

@jnothman
Copy link
Member Author

@lesteve or @amueller, merge?

@jnothman
Copy link
Member Author

Actually, we have sufficient consensus. Merging.

@jnothman jnothman merged commit f05a95b into scikit-learn:master Oct 16, 2017
jnothman added a commit to jnothman/scikit-learn that referenced this pull request Oct 20, 2017
@amueller
Copy link
Member

No one can have used ndcg_score

Can you explain that?

I'll go ahead with making release preparations, but this smells funny to me. I haven't kept track of the issues as you have, though...

@jnothman
Copy link
Member Author

jnothman commented Oct 22, 2017 via email

maskani-moh pushed a commit to maskani-moh/scikit-learn that referenced this pull request Nov 15, 2017
jwjohnson314 pushed a commit to jwjohnson314/scikit-learn that referenced this pull request Dec 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants