Skip to content

Changed Ranker to Ranking in evaluation related files. #2675

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 5 commits into from
Feb 25, 2019

Conversation

zeahmed
Copy link
Contributor

@zeahmed zeahmed commented Feb 21, 2019

This PR fixes #2634

Some of the EntryPoint names got changed. Reviewers' are requested to please verify consequences of changing EntryPoint names.

Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka left a comment

Choose a reason for hiding this comment

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

:shipit:

@codecov
Copy link

codecov bot commented Feb 21, 2019

Codecov Report

Merging #2675 into master will increase coverage by <.01%.
The diff coverage is 72.5%.

@@            Coverage Diff             @@
##           master    #2675      +/-   ##
==========================================
+ Coverage    71.7%    71.7%   +<.01%     
==========================================
  Files         808      808              
  Lines      142221   142221              
  Branches    16131    16131              
==========================================
+ Hits       101981   101982       +1     
- Misses      35799    35800       +1     
+ Partials     4441     4439       -2
Flag Coverage Δ
#Debug 71.7% <72.5%> (ø) ⬆️
#production 67.96% <80%> (ø) ⬆️
#test 85.83% <20%> (-0.02%) ⬇️
Impacted Files Coverage Δ
test/Microsoft.ML.Functional.Tests/Common.cs 98.55% <ø> (ø) ⬆️
src/Microsoft.ML.Data/EntryPoints/InputBuilder.cs 81.65% <ø> (ø) ⬆️
test/Microsoft.ML.Benchmarks/Numeric/Ranking.cs 0% <0%> (ø) ⬆️
src/Microsoft.ML.EntryPoints/MacroUtils.cs 88.46% <100%> (ø) ⬆️
...osoft.ML.Data/Evaluators/Metrics/RankingMetrics.cs 90.9% <100%> (ø)
...crosoft.ML.Core.Tests/UnitTests/TestEntryPoints.cs 98.07% <100%> (ø) ⬆️
...ansforms/PermutationFeatureImportanceExtensions.cs 99% <100%> (ø) ⬆️
...crosoft.ML.StaticPipe/EvaluatorStaticExtensions.cs 100% <100%> (ø) ⬆️
...c/Microsoft.ML.EntryPoints/CrossValidationMacro.cs 85.53% <100%> (ø) ⬆️
src/Microsoft.ML.Data/TrainCatalog.cs 90.08% <100%> (ø) ⬆️
... and 5 more

@@ -25,7 +25,7 @@ namespace Microsoft.ML
[BestFriend]
internal delegate void SignatureMultiOutputRegressorTrainer();
[BestFriend]
internal delegate void SignatureRankerTrainer();
internal delegate void SignatureRankingTrainer();
Copy link
Contributor

@TomFinley TomFinley Feb 21, 2019

Choose a reason for hiding this comment

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

SignatureRankingTrainer [](start = 27, length = 23)

Since this is internal it is not a big deal, but note that this is incorrect. So you see how it is SignatureBinaryClassifierTrainer above? This is because the thing that does the task binary classification is a binary classifier. So this should have been retained as signature ranker trainer. What is the task? Ranking. What is the thing doing the task? A ranker.

Sort of like how things are being transformed, and the thing that does this is called a transformer. The use of different tenses, casing, and whatnot is deliberate.

Not a huge deal, but if you have to post another iteration anyway, this is what I would call a hypercorrection that should be backcorrected. #Resolved

@@ -44,7 +44,7 @@ private static class DefaultEvaluatorTable
{ MetadataUtils.Const.ScoreColumnKind.Regression, env => new RegressionMamlEvaluator(env, new RegressionMamlEvaluator.Arguments()) },
{ MetadataUtils.Const.ScoreColumnKind.MultiOutputRegression, env => new MultiOutputRegressionMamlEvaluator(env, new MultiOutputRegressionMamlEvaluator.Arguments()) },
{ MetadataUtils.Const.ScoreColumnKind.QuantileRegression, env => new QuantileRegressionMamlEvaluator(env, new QuantileRegressionMamlEvaluator.Arguments()) },
{ MetadataUtils.Const.ScoreColumnKind.Ranking, env => new RankerMamlEvaluator(env, new RankerMamlEvaluator.Arguments()) },
{ MetadataUtils.Const.ScoreColumnKind.Ranking, env => new RankingMamlEvaluator(env, new RankingMamlEvaluator.Arguments()) },
Copy link
Contributor

@TomFinley TomFinley Feb 21, 2019

Choose a reason for hiding this comment

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

RankingMamlEvaluator [](start = 86, length = 20)

Note that this renaming is correct. You are not evaluating a ranker, you are evaluating a ranking, so this one is good. #Resolved

Copy link
Contributor

Choose a reason for hiding this comment

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

It is also you note now more consistent with its "brethren" here.


In reply to: 258754361 [](ancestors = 258754361)

@@ -22,7 +22,7 @@ public enum TrainerKinds
{
SignatureBinaryClassifierTrainer,
SignatureMultiClassClassifierTrainer,
SignatureRankerTrainer,
SignatureRankingTrainer,
Copy link
Contributor

@TomFinley TomFinley Feb 21, 2019

Choose a reason for hiding this comment

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

SignatureRankingTrainer [](start = 12, length = 23)

Another thing where this is I think hypercorrected. See how it is now inconsistent with everything else here? #Resolved

@TomFinley
Copy link
Contributor

TomFinley commented Feb 21, 2019

Another way we could go is change all these things into things like SignatureBinaryClassificationTrainer. That also seems like a defensible choice, but we'd want them all to be consistent. That strikes me as possibly more work than we want to do right now and its urgency does not seem clear. #Resolved

@zeahmed
Copy link
Contributor Author

zeahmed commented Feb 21, 2019

Sure! I am changing SignatureRankingTrainer back to SignatureRankerTrainer so that we don't have to change many things right now.


In reply to: 465837115 [](ancestors = 465837115)

@zeahmed zeahmed requested a review from Ivanidzo4ka February 21, 2019 22:30
@zeahmed
Copy link
Contributor Author

zeahmed commented Feb 21, 2019

@Ivanidzo4ka, I accidentally click the re-request-review icon next to the review name above which made me lost your review approval. Can you please approve it back? Thanks!

@Ivanidzo4ka
Copy link
Contributor

once it's gone, IT'S GONE!


In reply to: 466194690 [](ancestors = 466194690)

Copy link
Contributor

@TomFinley TomFinley left a comment

Choose a reason for hiding this comment

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

This change seems good to me, thank you @zeahmed .

@zeahmed
Copy link
Contributor Author

zeahmed commented Feb 25, 2019

@Ivanidzo4ka and @TomFinley, thanks for the useful comments and suggestions.

@zeahmed zeahmed merged commit 4420cc7 into dotnet:master Feb 25, 2019
@ghost ghost locked as resolved and limited conversation to collaborators Mar 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ranking / Ranker API inconsistency
3 participants