-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Added RankingEvaluatorOptions and removed the truncation limit. #4081
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
Changes from 9 commits
8749a10
fe25bf6
cb446be
b5ee220
b9a7471
80e238d
2ef424d
3958f01
56d4595
00bc7ef
d0462f1
87cefbc
c3a908b
c0a430a
0b55903
56983d5
3382d1d
8ca5d01
4ac459e
8f20ea4
f9f9e1d
21cb8f3
138f201
55e3460
e43bba3
421d713
4f4f81c
89082a5
f167af8
0d4d34f
6cd2f15
1424ab3
3ee03ca
34b7a91
5539127
02053a6
35ad3c0
0eb3e2b
a3291b1
37af437
68f1f35
5b90a34
0efe238
b6584aa
7d47832
0e99776
20a4490
0d111f4
72d1a4d
ea9ebed
013be4f
d2ae365
a9e6db8
5855f99
724bb12
d009f55
8f7b6cd
30d56a0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,8 +26,7 @@ | |
|
||
namespace Microsoft.ML.Data | ||
{ | ||
[BestFriend] | ||
internal sealed class RankingEvaluator : EvaluatorBase<RankingEvaluator.Aggregator> | ||
public sealed class RankingEvaluator : EvaluatorBase<RankingEvaluator.Aggregator> | ||
{ | ||
public sealed class Arguments | ||
{ | ||
|
@@ -271,7 +270,7 @@ public sealed class Aggregator : AggregatorBase | |
{ | ||
public sealed class Counters | ||
{ | ||
public const int MaxTruncationLevel = 10; | ||
public const int MaxTruncationLevel = 100; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a technical need for any fixed limit? From the data science side, I don't know of one. Is there a ramification in our code for simply removing the limit? #Resolved There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The original author of this code has assumed 100 to be a reasonable limit and that assumption is enforced in many places in the code. The MaxTruncationLevel of 10 set above made it a bit inconsistent and my fix just makes the max limit consistent. In theory it should be possible to remove the limit. But the way the code is structured makes it a bit more invasive than necessary. Right now, in
The way the code is structured, this requires some well defined maximum. It should be possible to remove this but that would require a bigger code change. I would like input on whether or the original author's assumption that 100 is a reasonable limit is a good assumption. If it is really necessary to not have a limit at all, it is a bigger code change. Can we take this fix as is and open a new bug to remove the limit and we can triage that separately? #Resolved There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As a datapoint, TensorFlow has no limit, and if the user doesn't specify It looks to be a pretty small fix to have no hard limit. Having no limit lets the user set their truncation level to say 500 should they want to. Currently the value of 10 assumes users won't care about the quality of their search results beyond the 1st page of results. Setting to 100 similarly assumes users won't care about the quality of results after page 10. #Resolved |
||
|
||
public readonly int TruncationLevel; | ||
private readonly List<Double[]> _groupNdcg; | ||
|
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is exposing the four classes the right method?
/cc @eerhardt #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No this is not the right approach. @harishsk - can you follow the same pattern we are following for the other evaluators? None of the other evaluator classes are public. There should be no reason why this is.
In reply to: 314017216 [](ancestors = 314017216)