Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2346263002:
Extending the UserClassifier to actually support classification. (Closed)
DescriptionExtending the UserClassifier to actually support classification.
This CL adds actual classification to the UserClassifier.
So far, only simple classification thresholds are implemented.
The CL also includes a new snippets-internals section and a bit of
refactoring.
In a follow-up CLs, all the parameters will become configurable by
variations.
BUG=648252
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/8048f8d8d5153a0e530529e883c78582853d0b15
Cr-Commit-Position: refs/heads/master@{#420022}
Patch Set 1 #
Total comments: 33
Patch Set 2 : Marc's comments #
Total comments: 4
Patch Set 3 : Minor rebase & larger refactoring #
Total comments: 33
Patch Set 4 : Comments of Tim and Marc #
Total comments: 17
Patch Set 5 : Marc's comments #
Total comments: 2
Patch Set 6 : Removing unnecessary static_casts #
Total comments: 8
Patch Set 7 : Bernhard's comments #
Messages
Total messages: 26 (10 generated)
Description was changed from ========== Extending the UserClassifier to actually support classification. This CL adds actual classification to the UserClassifier. So far, only simple classification thresholds are implemented. The CL also includes a new snippets-internals section and a bit of refactoring. In a follow-up CLs, all the parameters will become configurable by variations. BUG=648252 ========== to ========== Extending the UserClassifier to actually support classification. This CL adds actual classification to the UserClassifier. So far, only simple classification thresholds are implemented. The CL also includes a new snippets-internals section and a bit of refactoring. In a follow-up CLs, all the parameters will become configurable by variations. BUG=648252 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
jkrcal@chromium.org changed reviewers: + treib@chromium.org
Marc, could you PTAL?
Wow, math is hard :D https://codereview.chromium.org/2346263002/diff/1/components/ntp_snippets/use... File components/ntp_snippets/user_classifier.cc (right): https://codereview.chromium.org/2346263002/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier.cc:40: // Default frequency values for new users. Again, intervals, not frequencies https://codereview.chromium.org/2346263002/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier.cc:59: discount_rate_per_hour = (std::log(1 / (1 - kDiscountFactorPerDay)) / 24); nit: remove the extra set of parens. nit2: Can you write all the double constants with ".0"? IMO that makes it much easier to see that there's no unintentional integer arithmetic going on. And finally, probably making these nits obsolete: This function effectively returns a constant, right? Then why not just put that constant there directly. Maybe our compilers are by now good enough that you can just write const double kDiscountRatePerHour = ...; but if not, then compute it manually and put the formula in a comment. That would get rid of the kinda ugly static variable. https://codereview.chromium.org/2346263002/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier.cc:65: // Returns the new value of the metric using its |old_value|, assuming What's the unit of the metric? What does the returned number tell me? https://codereview.chromium.org/2346263002/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier.cc:125: GetMetricValueForEstimateHoursBetweenEvents( Hm, you're changing the defaults of existing prefs. For existing users, that won't do anything. I assume that won't break things? https://codereview.chromium.org/2346263002/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier.cc:184: nit: extra empty line (the other similar methods don't have it) https://codereview.chromium.org/2346263002/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier.cc:198: return UserClass::OCCASIONAL_NTP_USER; nit: Braces please https://codereview.chromium.org/2346263002/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier.cc:202: return UserClass::FREQUENT_NTP_USER; Also here https://codereview.chromium.org/2346263002/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier.cc:214: return "Frequent user of content suggestions"; This string is inconsistent with the others. https://codereview.chromium.org/2346263002/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier.cc:233: if (!pref_service_) Pre-existing, but: Can this ever happen? If so, please add a comment; otherwise remove the check. https://codereview.chromium.org/2346263002/diff/1/components/ntp_snippets/use... File components/ntp_snippets/user_classifier.h (right): https://codereview.chromium.org/2346263002/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier.h:48: // Get the estimate frequencies of events that the classifier tracks. nit: these return intervals, not frequencies. All of these should probably be const? https://codereview.chromium.org/2346263002/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier.h:55: std::string GetUserClassDescription(); Mark this "ForDebugging" or something like that? Otherwise people might expect the string to be translated etc. https://codereview.chromium.org/2346263002/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier.h:58: void ClearClassificationForTesting(); s/ForTesting/ForDebugging/ "ForTesting" implies it's only used in (unit) tests. https://codereview.chromium.org/2346263002/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier.h:64: const char* last_time_pref_name); misaligned
Thanks! PTAL, again! https://codereview.chromium.org/2346263002/diff/1/components/ntp_snippets/use... File components/ntp_snippets/user_classifier.cc (right): https://codereview.chromium.org/2346263002/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier.cc:40: // Default frequency values for new users. On 2016/09/19 15:20:16, Marc Treib wrote: > Again, intervals, not frequencies Done. https://codereview.chromium.org/2346263002/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier.cc:59: discount_rate_per_hour = (std::log(1 / (1 - kDiscountFactorPerDay)) / 24); On 2016/09/19 15:20:16, Marc Treib wrote: > nit: remove the extra set of parens. > nit2: Can you write all the double constants with ".0"? IMO that makes it much > easier to see that there's no unintentional integer arithmetic going on. > > And finally, probably making these nits obsolete: This function effectively > returns a constant, right? Then why not just put that constant there directly. > Maybe our compilers are by now good enough that you can just write > const double kDiscountRatePerHour = ...; > but if not, then compute it manually and put the formula in a comment. That > would get rid of the kinda ugly static variable. Done the nits. In a next CL, I want to override it by a variation param which I cannot do the way you propose. Do you see any nicer way than a static variable? (I need to keep it in a static function because it is accessed from the static member function.) https://codereview.chromium.org/2346263002/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier.cc:65: // Returns the new value of the metric using its |old_value|, assuming On 2016/09/19 15:20:15, Marc Treib wrote: > What's the unit of the metric? What does the returned number tell me? Hmm :) Nothing intuitive. I have added a comment in the beginning of the private "implementation" part of the header file. Did it alleviate your concerns? https://codereview.chromium.org/2346263002/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier.cc:125: GetMetricValueForEstimateHoursBetweenEvents( On 2016/09/19 15:20:16, Marc Treib wrote: > Hm, you're changing the defaults of existing prefs. For existing users, that > won't do anything. I assume that won't break things? I think the default values are not stored anywhere, always registered on runtime. When I changed the default values here, and cleared the stored values by the "Clear..." function below, I got the new defaults applied. So it sounds safe to me. https://codereview.chromium.org/2346263002/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier.cc:184: On 2016/09/19 15:20:16, Marc Treib wrote: > nit: extra empty line (the other similar methods don't have it) Done. https://codereview.chromium.org/2346263002/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier.cc:198: return UserClass::OCCASIONAL_NTP_USER; On 2016/09/19 15:20:16, Marc Treib wrote: > nit: Braces please Done. https://codereview.chromium.org/2346263002/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier.cc:202: return UserClass::FREQUENT_NTP_USER; On 2016/09/19 15:20:16, Marc Treib wrote: > Also here Done. https://codereview.chromium.org/2346263002/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier.cc:214: return "Frequent user of content suggestions"; On 2016/09/19 15:20:16, Marc Treib wrote: > This string is inconsistent with the others. Done. https://codereview.chromium.org/2346263002/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier.cc:233: if (!pref_service_) On 2016/09/19 15:20:16, Marc Treib wrote: > Pre-existing, but: Can this ever happen? If so, please add a comment; otherwise > remove the check. Done. https://codereview.chromium.org/2346263002/diff/1/components/ntp_snippets/use... File components/ntp_snippets/user_classifier.h (right): https://codereview.chromium.org/2346263002/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier.h:48: // Get the estimate frequencies of events that the classifier tracks. On 2016/09/19 15:20:16, Marc Treib wrote: > nit: these return intervals, not frequencies. > > All of these should probably be const? Done. https://codereview.chromium.org/2346263002/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier.h:55: std::string GetUserClassDescription(); On 2016/09/19 15:20:16, Marc Treib wrote: > Mark this "ForDebugging" or something like that? Otherwise people might expect > the string to be translated etc. Done. https://codereview.chromium.org/2346263002/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier.h:58: void ClearClassificationForTesting(); On 2016/09/19 15:20:16, Marc Treib wrote: > s/ForTesting/ForDebugging/ > > "ForTesting" implies it's only used in (unit) tests. Done. https://codereview.chromium.org/2346263002/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier.h:64: const char* last_time_pref_name); On 2016/09/19 15:20:16, Marc Treib wrote: > misaligned Done.
Marc, I did not like the code repetition for the 3 different metrics. So I could not resist refactoring a bit. PTAL!
https://codereview.chromium.org/2346263002/diff/1/components/ntp_snippets/use... File components/ntp_snippets/user_classifier.cc (right): https://codereview.chromium.org/2346263002/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier.cc:59: discount_rate_per_hour = (std::log(1 / (1 - kDiscountFactorPerDay)) / 24); On 2016/09/19 18:45:25, jkrcal wrote: > On 2016/09/19 15:20:16, Marc Treib wrote: > > nit: remove the extra set of parens. > > nit2: Can you write all the double constants with ".0"? IMO that makes it much > > easier to see that there's no unintentional integer arithmetic going on. > > > > And finally, probably making these nits obsolete: This function effectively > > returns a constant, right? Then why not just put that constant there directly. > > Maybe our compilers are by now good enough that you can just write > > const double kDiscountRatePerHour = ...; > > but if not, then compute it manually and put the formula in a comment. That > > would get rid of the kinda ugly static variable. > > Done the nits. > > In a next CL, I want to override it by a variation param which I cannot do the > way you propose. Do you see any nicer way than a static variable? (I need to > keep it in a static function because it is accessed from the static member > function.) And you want the variation param to specify the per-day value rather than the per-hour value? I'd say that it doesn't really matter which of those we specify in the config. If you want to keep the per-day value in the config: One way to get rid of the static var would be to just compute on every call :) https://codereview.chromium.org/2346263002/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier.cc:65: // Returns the new value of the metric using its |old_value|, assuming On 2016/09/19 18:45:25, jkrcal wrote: > On 2016/09/19 15:20:15, Marc Treib wrote: > > What's the unit of the metric? What does the returned number tell me? > > Hmm :) Nothing intuitive. I have added a comment in the beginning of the private > "implementation" part of the header file. Did it alleviate your concerns? Yup, that helps, thanks! https://codereview.chromium.org/2346263002/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier.cc:125: GetMetricValueForEstimateHoursBetweenEvents( On 2016/09/19 18:45:25, jkrcal wrote: > On 2016/09/19 15:20:16, Marc Treib wrote: > > Hm, you're changing the defaults of existing prefs. For existing users, that > > won't do anything. I assume that won't break things? > > I think the default values are not stored anywhere, always registered on > runtime. When I changed the default values here, and cleared the stored values > by the "Clear..." function below, I got the new defaults applied. > > So it sounds safe to me. Yes, I mostly meant that users who used M54 briefly will have "wrong" values when they switch to M55. But that's probably okay. https://codereview.chromium.org/2346263002/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier.cc:266: SetLastTimeToNow(last_time_pref_name); Was the reason for moving this out only so you can make the method const? https://codereview.chromium.org/2346263002/diff/20001/components/ntp_snippets... File components/ntp_snippets/user_classifier.cc (right): https://codereview.chromium.org/2346263002/diff/20001/components/ntp_snippets... components/ntp_snippets/user_classifier.cc:242: if (!hours_since_last_time) This will check for zero - is that what you want? Doesn't seem very safe... https://codereview.chromium.org/2346263002/diff/20001/components/ntp_snippets... File components/ntp_snippets/user_classifier.h (right): https://codereview.chromium.org/2346263002/diff/20001/components/ntp_snippets... components/ntp_snippets/user_classifier.h:63: // event, we keep a simple frequency model, a single value that we call s/event/events/ https://codereview.chromium.org/2346263002/diff/40001/components/ntp_snippets... File components/ntp_snippets/user_classifier.cc (right): https://codereview.chromium.org/2346263002/diff/40001/components/ntp_snippets... components/ntp_snippets/user_classifier.cc:59: const double kDefaults[] = {24, 36, 48}; optional: add some static_asserts to make sure these all have the correct size? (Probably by adding a Metric::COUNT) https://codereview.chromium.org/2346263002/diff/40001/components/ntp_snippets... components/ntp_snippets/user_classifier.cc:63: GetDiscountRatePerHour() { extra line break https://codereview.chromium.org/2346263002/diff/40001/components/ntp_snippets... components/ntp_snippets/user_classifier.cc:128: const UserClassifier::Metric UserClassifier::kMetrics[3] = { Is the "3" required? https://codereview.chromium.org/2346263002/diff/40001/components/ntp_snippets... components/ntp_snippets/user_classifier.cc:180: double UserClassifier::GetEstimatedAvgTimeToOpenNTP() const { Are the three GetEstimatedAvgTimeTo... methods still useful? Looks like we might just as well call GetEstimatedAvgTime(Metric::...) https://codereview.chromium.org/2346263002/diff/40001/components/ntp_snippets... components/ntp_snippets/user_classifier.cc:250: true /* event_now */); nit: Preferred style is /*event_now=*/true https://codereview.chromium.org/2346263002/diff/40001/components/ntp_snippets... components/ntp_snippets/user_classifier.cc:280: DCHECK(pref_service_); These DCHECKs aren't really helpful: If it were null, we'd crash on the next line anyway. And it's obvious to the reader that it must not be null, so it also doesn't provide documentation. https://codereview.chromium.org/2346263002/diff/40001/components/ntp_snippets... File components/ntp_snippets/user_classifier.h (right): https://codereview.chromium.org/2346263002/diff/40001/components/ntp_snippets... components/ntp_snippets/user_classifier.h:71: enum Metric { OPEN_NTP = 0, SHOW_SUGGESTIONS, USE_SUGGESTIONS }; enum class? You're using it like "Metric::..." anyway. https://codereview.chromium.org/2346263002/diff/40001/components/ntp_snippets... components/ntp_snippets/user_classifier.h:72: static const Metric kMetrics[3]; Could you just define this in the .cc? https://codereview.chromium.org/2346263002/diff/40001/components/ntp_snippets... components/ntp_snippets/user_classifier.h:76: double GetEstimatedAvgTime(const Metric metric) const; nit: "const Metric" doesn't do anything here - you're passing by value anyway.
some naming nits ;-) https://codereview.chromium.org/2346263002/diff/40001/components/ntp_snippets... File components/ntp_snippets/user_classifier.cc (right): https://codereview.chromium.org/2346263002/diff/40001/components/ntp_snippets... components/ntp_snippets/user_classifier.cc:66: if (discount_rate_per_hour == 0.0) { what's the purpose of this? Do you want to initialize it only once? If so, put the line into the initialization. If not, comparing floating point values against 0.0 is tricky; you should compare against some epsilon instead. https://codereview.chromium.org/2346263002/diff/40001/components/ntp_snippets... components/ntp_snippets/user_classifier.cc:95: if (metric_value <= 1) maybe adjust the comment to also explain why this is necessary (i.e. avoid the division by zero and taking the logarithm of negative values). IMO, that's the more important bit. https://codereview.chromium.org/2346263002/diff/40001/components/ntp_snippets... File components/ntp_snippets/user_classifier.h (right): https://codereview.chromium.org/2346263002/diff/40001/components/ntp_snippets... components/ntp_snippets/user_classifier.h:25: OCCASIONAL_NTP_USER, i wonder if 'occasional' isn't too generous. This would be the bucket of users we don't really expect to interact with our suggestions. Should we name this 'RARE_NTP_USERS'? Also, what would be a qualified term for 'NORMAL_NTP_USER'? That name gives the impression that we observed the whole population of chrome users and defined a "NORMAL_NTP_USER" class based on statistics... https://codereview.chromium.org/2346263002/diff/40001/components/ntp_snippets... components/ntp_snippets/user_classifier.h:71: enum Metric { OPEN_NTP = 0, SHOW_SUGGESTIONS, USE_SUGGESTIONS }; On 2016/09/20 10:27:03, Marc Treib wrote: > enum class? You're using it like "Metric::..." anyway. naming nit: these constants sound like they would be describing actions. I think you want to describe events (rename the enum?); a consistent naming would be NTP_OPENED, SUGGESTIONS_SHOWN, SUGGESTIONS_USED. Or NTP_IMPRESSION, SUGGESTIONS_IMPRESSION, SUGGESTIONS_INTERACTION if you prefer nouns.
Thanks, guys! PTAL, again. https://codereview.chromium.org/2346263002/diff/1/components/ntp_snippets/use... File components/ntp_snippets/user_classifier.cc (right): https://codereview.chromium.org/2346263002/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier.cc:59: discount_rate_per_hour = (std::log(1 / (1 - kDiscountFactorPerDay)) / 24); On 2016/09/20 10:27:02, Marc Treib wrote: > On 2016/09/19 18:45:25, jkrcal wrote: > > On 2016/09/19 15:20:16, Marc Treib wrote: > > > nit: remove the extra set of parens. > > > nit2: Can you write all the double constants with ".0"? IMO that makes it > much > > > easier to see that there's no unintentional integer arithmetic going on. > > > > > > And finally, probably making these nits obsolete: This function effectively > > > returns a constant, right? Then why not just put that constant there > directly. > > > Maybe our compilers are by now good enough that you can just write > > > const double kDiscountRatePerHour = ...; > > > but if not, then compute it manually and put the formula in a comment. That > > > would get rid of the kinda ugly static variable. > > > > Done the nits. > > > > In a next CL, I want to override it by a variation param which I cannot do the > > way you propose. Do you see any nicer way than a static variable? (I need to > > keep it in a static function because it is accessed from the static member > > function.) > > And you want the variation param to specify the per-day value rather than the > per-hour value? I'd say that it doesn't really matter which of those we specify > in the config. > > If you want to keep the per-day value in the config: One way to get rid of the > static var would be to just compute on every call :) I think the per-day value is better because it is much easier to assess (without going to your favorite math tool) than the per-hour value. Anyway, I've gotten rid of the static variable, in the end :) https://codereview.chromium.org/2346263002/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier.cc:125: GetMetricValueForEstimateHoursBetweenEvents( On 2016/09/20 10:27:02, Marc Treib wrote: > On 2016/09/19 18:45:25, jkrcal wrote: > > On 2016/09/19 15:20:16, Marc Treib wrote: > > > Hm, you're changing the defaults of existing prefs. For existing users, that > > > won't do anything. I assume that won't break things? > > > > I think the default values are not stored anywhere, always registered on > > runtime. When I changed the default values here, and cleared the stored values > > by the "Clear..." function below, I got the new defaults applied. > > > > So it sounds safe to me. > > Yes, I mostly meant that users who used M54 briefly will have "wrong" values > when they switch to M55. But that's probably okay. Not a big deal, IMO. The initial value has after a couple of days only a small influence on the metric value (25% of the value after 5 days, 13% after a week, 5% after 10 days). https://codereview.chromium.org/2346263002/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier.cc:266: SetLastTimeToNow(last_time_pref_name); On 2016/09/20 10:27:02, Marc Treib wrote: > Was the reason for moving this out only so you can make the method const? Mostly. I also think that it is clearer if a "Get" function does not store anything. There is a check in the constructor that replaces the functionality. https://codereview.chromium.org/2346263002/diff/20001/components/ntp_snippets... File components/ntp_snippets/user_classifier.cc (right): https://codereview.chromium.org/2346263002/diff/20001/components/ntp_snippets... components/ntp_snippets/user_classifier.cc:242: if (!hours_since_last_time) On 2016/09/20 10:27:02, Marc Treib wrote: > This will check for zero - is that what you want? Doesn't seem very safe... I agree, this a bit obscure, to say the least :) Not in the code anymore. https://codereview.chromium.org/2346263002/diff/20001/components/ntp_snippets... File components/ntp_snippets/user_classifier.h (right): https://codereview.chromium.org/2346263002/diff/20001/components/ntp_snippets... components/ntp_snippets/user_classifier.h:63: // event, we keep a simple frequency model, a single value that we call On 2016/09/20 10:27:02, Marc Treib wrote: > s/event/events/ Done. https://codereview.chromium.org/2346263002/diff/40001/components/ntp_snippets... File components/ntp_snippets/user_classifier.cc (right): https://codereview.chromium.org/2346263002/diff/40001/components/ntp_snippets... components/ntp_snippets/user_classifier.cc:59: const double kDefaults[] = {24, 36, 48}; On 2016/09/20 10:27:03, Marc Treib wrote: > optional: add some static_asserts to make sure these all have the correct size? > (Probably by adding a Metric::COUNT) Done. https://codereview.chromium.org/2346263002/diff/40001/components/ntp_snippets... components/ntp_snippets/user_classifier.cc:63: GetDiscountRatePerHour() { On 2016/09/20 10:27:03, Marc Treib wrote: > extra line break Done. https://codereview.chromium.org/2346263002/diff/40001/components/ntp_snippets... components/ntp_snippets/user_classifier.cc:66: if (discount_rate_per_hour == 0.0) { On 2016/09/20 11:32:47, tschumann wrote: > what's the purpose of this? > Do you want to initialize it only once? > If so, put the line into the initialization. If not, comparing floating point > values against 0.0 is tricky; you should compare against some epsilon instead. It was initialization, I removed the static var, after Marc's comments. https://codereview.chromium.org/2346263002/diff/40001/components/ntp_snippets... components/ntp_snippets/user_classifier.cc:95: if (metric_value <= 1) On 2016/09/20 11:32:47, tschumann wrote: > maybe adjust the comment to also explain why this is necessary (i.e. avoid the > division by zero and taking the logarithm of negative values). IMO, that's the > more important bit. Done. https://codereview.chromium.org/2346263002/diff/40001/components/ntp_snippets... components/ntp_snippets/user_classifier.cc:128: const UserClassifier::Metric UserClassifier::kMetrics[3] = { On 2016/09/20 10:27:03, Marc Treib wrote: > Is the "3" required? Done. https://codereview.chromium.org/2346263002/diff/40001/components/ntp_snippets... components/ntp_snippets/user_classifier.cc:180: double UserClassifier::GetEstimatedAvgTimeToOpenNTP() const { On 2016/09/20 10:27:03, Marc Treib wrote: > Are the three GetEstimatedAvgTimeTo... methods still useful? Looks like we might > just as well call GetEstimatedAvgTime(Metric::...) Done. https://codereview.chromium.org/2346263002/diff/40001/components/ntp_snippets... components/ntp_snippets/user_classifier.cc:250: true /* event_now */); On 2016/09/20 10:27:03, Marc Treib wrote: > nit: Preferred style is /*event_now=*/true I recently had a CL discussion with Bernhard: - Bernhard: Could you add spaces between the comment delimiters? This all looks a bit cramped. - Me: By chromium code search, /*var_name=*/value seems the way more popular way to format it. Do you insist? - Berhnard: Actually, what seems to be the most common style is `value /* variable_name */`, which I would not be averse to :) I do not care which of the formats I learn. I just do want them to be reviewer-independent :) https://codereview.chromium.org/2346263002/diff/40001/components/ntp_snippets... components/ntp_snippets/user_classifier.cc:280: DCHECK(pref_service_); On 2016/09/20 10:27:03, Marc Treib wrote: > These DCHECKs aren't really helpful: If it were null, we'd crash on the next > line anyway. And it's obvious to the reader that it must not be null, so it also > doesn't provide documentation. Done. https://codereview.chromium.org/2346263002/diff/40001/components/ntp_snippets... File components/ntp_snippets/user_classifier.h (right): https://codereview.chromium.org/2346263002/diff/40001/components/ntp_snippets... components/ntp_snippets/user_classifier.h:25: OCCASIONAL_NTP_USER, On 2016/09/20 11:32:47, tschumann wrote: > i wonder if 'occasional' isn't too generous. > This would be the bucket of users we don't really expect to interact with our > suggestions. Should we name this 'RARE_NTP_USERS'? > Also, what would be a qualified term for 'NORMAL_NTP_USER'? That name gives the > impression that we observed the whole population of chrome users and defined a > "NORMAL_NTP_USER" class based on statistics... Done - RARE. Replace NORMAL by ORDINARY. Have really spent some time thinking, could not come up with anything better. WDYT? https://codereview.chromium.org/2346263002/diff/40001/components/ntp_snippets... components/ntp_snippets/user_classifier.h:71: enum Metric { OPEN_NTP = 0, SHOW_SUGGESTIONS, USE_SUGGESTIONS }; On 2016/09/20 10:27:03, Marc Treib wrote: > enum class? You're using it like "Metric::..." anyway. Marc, if I make it an enum class, it forces me to use static_cast<int>() in the code which makes it look more cluttered, IMO. What is the advantage? https://codereview.chromium.org/2346263002/diff/40001/components/ntp_snippets... components/ntp_snippets/user_classifier.h:71: enum Metric { OPEN_NTP = 0, SHOW_SUGGESTIONS, USE_SUGGESTIONS }; On 2016/09/20 11:32:47, tschumann wrote: > On 2016/09/20 10:27:03, Marc Treib wrote: > > enum class? You're using it like "Metric::..." anyway. > > naming nit: these constants sound like they would be describing actions. I think > you want to describe events (rename the enum?); a consistent naming would be > NTP_OPENED, SUGGESTIONS_SHOWN, SUGGESTIONS_USED. > Or > NTP_IMPRESSION, SUGGESTIONS_IMPRESSION, SUGGESTIONS_INTERACTION if you prefer > nouns. Tim, done. https://codereview.chromium.org/2346263002/diff/40001/components/ntp_snippets... components/ntp_snippets/user_classifier.h:72: static const Metric kMetrics[3]; On 2016/09/20 10:27:03, Marc Treib wrote: > Could you just define this in the .cc? Done. (I needed to make the Metric enum public which was anyway useful for other changes you proposed.) https://codereview.chromium.org/2346263002/diff/40001/components/ntp_snippets... components/ntp_snippets/user_classifier.h:76: double GetEstimatedAvgTime(const Metric metric) const; On 2016/09/20 10:27:03, Marc Treib wrote: > nit: "const Metric" doesn't do anything here - you're passing by value anyway. Done.
Thanks! LGTM with a few more optional suggestions. https://codereview.chromium.org/2346263002/diff/40001/components/ntp_snippets... File components/ntp_snippets/user_classifier.cc (right): https://codereview.chromium.org/2346263002/diff/40001/components/ntp_snippets... components/ntp_snippets/user_classifier.cc:250: true /* event_now */); On 2016/09/20 13:10:13, jkrcal wrote: > On 2016/09/20 10:27:03, Marc Treib wrote: > > nit: Preferred style is /*event_now=*/true > > I recently had a CL discussion with Bernhard: > > - Bernhard: Could you add spaces between the comment delimiters? This all looks > a bit cramped. > - Me: By chromium code search, /*var_name=*/value seems the way more popular > way to format it. Do you insist? > - Berhnard: Actually, what seems to be the most common style is `value /* > variable_name */`, which I would not be averse to :) > > > I do not care which of the formats I learn. I just do want them to be > reviewer-independent :) Tim suggested the above format, because for internal code there is a tool that checks that the string matches the actual parameter name. Even though we don't have that in Chromium, the hope would be to get it at some point. Anyway, not a big deal. Carry on. :) https://codereview.chromium.org/2346263002/diff/40001/components/ntp_snippets... File components/ntp_snippets/user_classifier.h (right): https://codereview.chromium.org/2346263002/diff/40001/components/ntp_snippets... components/ntp_snippets/user_classifier.h:25: OCCASIONAL_NTP_USER, On 2016/09/20 13:10:13, jkrcal wrote: > On 2016/09/20 11:32:47, tschumann wrote: > > i wonder if 'occasional' isn't too generous. > > This would be the bucket of users we don't really expect to interact with our > > suggestions. Should we name this 'RARE_NTP_USERS'? > > Also, what would be a qualified term for 'NORMAL_NTP_USER'? That name gives > the > > impression that we observed the whole population of chrome users and defined a > > "NORMAL_NTP_USER" class based on statistics... > > Done - RARE. > Replace NORMAL by ORDINARY. Have really spent some time thinking, could not come > up with anything better. WDYT? MODERATE? https://codereview.chromium.org/2346263002/diff/40001/components/ntp_snippets... components/ntp_snippets/user_classifier.h:71: enum Metric { OPEN_NTP = 0, SHOW_SUGGESTIONS, USE_SUGGESTIONS }; On 2016/09/20 13:10:14, jkrcal wrote: > On 2016/09/20 10:27:03, Marc Treib wrote: > > enum class? You're using it like "Metric::..." anyway. > > Marc, if I make it an enum class, it forces me to use static_cast<int>() in the > code which makes it look more cluttered, IMO. What is the advantage? E.g. you couldn't access the members without the "Metric::" scope specification. The non-class enum can be used as "UserClassifier::NTP_OPENED" etc. Arguably the explicit casts to int are also an advantage ;) That said, it's not a big deal, and I'll leave the decision to you. https://codereview.chromium.org/2346263002/diff/60001/chrome/browser/android/... File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right): https://codereview.chromium.org/2346263002/diff/60001/chrome/browser/android/... chrome/browser/android/ntp/ntp_snippets_bridge.cc:272: ntp_snippets::UserClassifier::Metric::NTP_OPENED); misaligned https://codereview.chromium.org/2346263002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right): https://codereview.chromium.org/2346263002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/snippets_internals_message_handler.cc:319: ntp_snippets::UserClassifier::Metric::NTP_OPENED)), optional: You could add a "using ntp_snippets::UserClassifier;" above to make this a bit easier to read. https://codereview.chromium.org/2346263002/diff/60001/components/ntp_snippets... File components/ntp_snippets/user_classifier.cc (right): https://codereview.chromium.org/2346263002/diff/60001/components/ntp_snippets... components/ntp_snippets/user_classifier.cc:183: break; NOTREACHED() ? https://codereview.chromium.org/2346263002/diff/60001/components/ntp_snippets... components/ntp_snippets/user_classifier.cc:188: DCHECK(0 <= metric && metric < Metric::COUNT); IMO "DCHECK_NE(metric, Metric::COUNT)" would be enough (and a bit easier to read). It's generally assumed that the value you get will be a valid enum entry. https://codereview.chromium.org/2346263002/diff/60001/components/ntp_snippets... components/ntp_snippets/user_classifier.cc:212: return "Normal user of the NTP"; "Ordinary" (or whatever it becomes), to match the enum? https://codereview.chromium.org/2346263002/diff/60001/components/ntp_snippets... File components/ntp_snippets/user_classifier.h (right): https://codereview.chromium.org/2346263002/diff/60001/components/ntp_snippets... components/ntp_snippets/user_classifier.h:42: NTP_OPENED = 0, // When the user opens a new NTP - this indicates potential Any particular reason for the "= 0"? It's the default, and doesn't seem important anyway. https://codereview.chromium.org/2346263002/diff/60001/components/ntp_snippets... components/ntp_snippets/user_classifier.h:59: void OnEvent(Metric metric); Add a comment? https://codereview.chromium.org/2346263002/diff/60001/components/ntp_snippets... components/ntp_snippets/user_classifier.h:75: // return the new value. Remove the extra line break https://codereview.chromium.org/2346263002/diff/60001/components/ntp_snippets... components/ntp_snippets/user_classifier.h:93: double discount_rate_per_hour_; You could make this const.
jkrcal@chromium.org changed reviewers: + bauerb@chromium.org
Marc, Tim, thanks again! Bernhard, could you PTAL at the snippets_internals files? https://codereview.chromium.org/2346263002/diff/40001/components/ntp_snippets... File components/ntp_snippets/user_classifier.cc (right): https://codereview.chromium.org/2346263002/diff/40001/components/ntp_snippets... components/ntp_snippets/user_classifier.cc:250: true /* event_now */); On 2016/09/20 13:26:52, Marc Treib wrote: > On 2016/09/20 13:10:13, jkrcal wrote: > > On 2016/09/20 10:27:03, Marc Treib wrote: > > > nit: Preferred style is /*event_now=*/true > > > > I recently had a CL discussion with Bernhard: > > > > - Bernhard: Could you add spaces between the comment delimiters? This all > looks > > a bit cramped. > > - Me: By chromium code search, /*var_name=*/value seems the way more popular > > way to format it. Do you insist? > > - Berhnard: Actually, what seems to be the most common style is `value /* > > variable_name */`, which I would not be averse to :) > > > > > > I do not care which of the formats I learn. I just do want them to be > > reviewer-independent :) > > Tim suggested the above format, because for internal code there is a tool that > checks that the string matches the actual parameter name. Even though we don't > have that in Chromium, the hope would be to get it at some point. > Anyway, not a big deal. Carry on. :) Done. https://codereview.chromium.org/2346263002/diff/40001/components/ntp_snippets... File components/ntp_snippets/user_classifier.h (right): https://codereview.chromium.org/2346263002/diff/40001/components/ntp_snippets... components/ntp_snippets/user_classifier.h:25: OCCASIONAL_NTP_USER, On 2016/09/20 13:26:52, Marc Treib wrote: > On 2016/09/20 13:10:13, jkrcal wrote: > > On 2016/09/20 11:32:47, tschumann wrote: > > > i wonder if 'occasional' isn't too generous. > > > This would be the bucket of users we don't really expect to interact with > our > > > suggestions. Should we name this 'RARE_NTP_USERS'? > > > Also, what would be a qualified term for 'NORMAL_NTP_USER'? That name gives > > the > > > impression that we observed the whole population of chrome users and defined > a > > > "NORMAL_NTP_USER" class based on statistics... > > > > Done - RARE. > > Replace NORMAL by ORDINARY. Have really spent some time thinking, could not > come > > up with anything better. WDYT? > > MODERATE? Renamed differently after an offline discussion with Tim. https://codereview.chromium.org/2346263002/diff/40001/components/ntp_snippets... components/ntp_snippets/user_classifier.h:71: enum Metric { OPEN_NTP = 0, SHOW_SUGGESTIONS, USE_SUGGESTIONS }; On 2016/09/20 13:26:52, Marc Treib wrote: > On 2016/09/20 13:10:14, jkrcal wrote: > > On 2016/09/20 10:27:03, Marc Treib wrote: > > > enum class? You're using it like "Metric::..." anyway. > > > > Marc, if I make it an enum class, it forces me to use static_cast<int>() in > the > > code which makes it look more cluttered, IMO. What is the advantage? > > E.g. you couldn't access the members without the "Metric::" scope specification. > The non-class enum can be used as "UserClassifier::NTP_OPENED" etc. > Arguably the explicit casts to int are also an advantage ;) > That said, it's not a big deal, and I'll leave the decision to you. Done. https://codereview.chromium.org/2346263002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right): https://codereview.chromium.org/2346263002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/snippets_internals_message_handler.cc:319: ntp_snippets::UserClassifier::Metric::NTP_OPENED)), On 2016/09/20 13:26:52, Marc Treib wrote: > optional: You could add a "using ntp_snippets::UserClassifier;" above to make > this a bit easier to read. Done. https://codereview.chromium.org/2346263002/diff/60001/components/ntp_snippets... File components/ntp_snippets/user_classifier.cc (right): https://codereview.chromium.org/2346263002/diff/60001/components/ntp_snippets... components/ntp_snippets/user_classifier.cc:183: break; On 2016/09/20 13:26:52, Marc Treib wrote: > NOTREACHED() ? Done. https://codereview.chromium.org/2346263002/diff/60001/components/ntp_snippets... components/ntp_snippets/user_classifier.cc:188: DCHECK(0 <= metric && metric < Metric::COUNT); On 2016/09/20 13:26:52, Marc Treib wrote: > IMO "DCHECK_NE(metric, Metric::COUNT)" would be enough (and a bit easier to > read). It's generally assumed that the value you get will be a valid enum entry. Done. Are enum values always non-negative? https://codereview.chromium.org/2346263002/diff/60001/components/ntp_snippets... components/ntp_snippets/user_classifier.cc:212: return "Normal user of the NTP"; On 2016/09/20 13:26:52, Marc Treib wrote: > "Ordinary" (or whatever it becomes), to match the enum? Done. https://codereview.chromium.org/2346263002/diff/60001/components/ntp_snippets... File components/ntp_snippets/user_classifier.h (right): https://codereview.chromium.org/2346263002/diff/60001/components/ntp_snippets... components/ntp_snippets/user_classifier.h:42: NTP_OPENED = 0, // When the user opens a new NTP - this indicates potential On 2016/09/20 13:26:52, Marc Treib wrote: > Any particular reason for the "= 0"? It's the default, and doesn't seem > important anyway. Done. https://codereview.chromium.org/2346263002/diff/60001/components/ntp_snippets... components/ntp_snippets/user_classifier.h:59: void OnEvent(Metric metric); On 2016/09/20 13:26:52, Marc Treib wrote: > Add a comment? Done. https://codereview.chromium.org/2346263002/diff/60001/components/ntp_snippets... components/ntp_snippets/user_classifier.h:75: // return the new value. On 2016/09/20 13:26:52, Marc Treib wrote: > Remove the extra line break Done. https://codereview.chromium.org/2346263002/diff/60001/components/ntp_snippets... components/ntp_snippets/user_classifier.h:93: double discount_rate_per_hour_; On 2016/09/20 13:26:53, Marc Treib wrote: > You could make this const. Done.
https://codereview.chromium.org/2346263002/diff/80001/components/ntp_snippets... File components/ntp_snippets/user_classifier.cc (right): https://codereview.chromium.org/2346263002/diff/80001/components/ntp_snippets... components/ntp_snippets/user_classifier.cc:169: DCHECK_NE(static_cast<int>(metric), static_cast<int>(Metric::COUNT)); Ah, the casts are required because the enum class has no operator<<? "DCHECK(metric != Metric::COUNT)" would also work, whichever you prefer.
Thanks, Marc. Bernhard, PTAL. https://codereview.chromium.org/2346263002/diff/80001/components/ntp_snippets... File components/ntp_snippets/user_classifier.cc (right): https://codereview.chromium.org/2346263002/diff/80001/components/ntp_snippets... components/ntp_snippets/user_classifier.cc:169: DCHECK_NE(static_cast<int>(metric), static_cast<int>(Metric::COUNT)); On 2016/09/20 13:53:42, Marc Treib wrote: > Ah, the casts are required because the enum class has no operator<<? > "DCHECK(metric != Metric::COUNT)" would also work, whichever you prefer. Ah, my mistake, the casts are not required. Thanks!
LGTM w/ some nits: https://codereview.chromium.org/2346263002/diff/40001/components/ntp_snippets... File components/ntp_snippets/user_classifier.cc (right): https://codereview.chromium.org/2346263002/diff/40001/components/ntp_snippets... components/ntp_snippets/user_classifier.cc:250: true /* event_now */); On 2016/09/20 13:26:52, Marc Treib wrote: > On 2016/09/20 13:10:13, jkrcal wrote: > > On 2016/09/20 10:27:03, Marc Treib wrote: > > > nit: Preferred style is /*event_now=*/true > > > > I recently had a CL discussion with Bernhard: > > > > - Bernhard: Could you add spaces between the comment delimiters? This all > looks > > a bit cramped. > > - Me: By chromium code search, /*var_name=*/value seems the way more popular > > way to format it. Do you insist? > > - Berhnard: Actually, what seems to be the most common style is `value /* > > variable_name */`, which I would not be averse to :) > > > > > > I do not care which of the formats I learn. I just do want them to be > > reviewer-independent :) > > Tim suggested the above format, because for internal code there is a tool that > checks that the string matches the actual parameter name. Even though we don't > have that in Chromium, the hope would be to get it at some point. > Anyway, not a big deal. Carry on. :) Weeeelll… If that tool needs to be written / ported for Chromium anyway, we might be able to teach it both styles :) https://codereview.chromium.org/2346263002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/snippets_internals.html (right): https://codereview.chromium.org/2346263002/diff/100001/chrome/browser/resourc... chrome/browser/resources/snippets_internals.html:67: <input id="clear-classification" type="submit" Nit: I think the <button> tag might be a bit more idiomatic for this (using a submit button technically also gives you a button, but I'm pretty sure dates back to when HTML was mostly static pages and forms). (And yeah, that also applies to the other existing buttons in here. Sorry, I've missed that previously.) https://codereview.chromium.org/2346263002/diff/100001/components/ntp_snippet... File components/ntp_snippets/user_classifier.cc (right): https://codereview.chromium.org/2346263002/diff/100001/components/ntp_snippet... components/ntp_snippets/user_classifier.cc:68: static_cast<int>(UserClassifier::Metric::COUNT) && Maybe split this up into separate asserts? https://codereview.chromium.org/2346263002/diff/100001/components/ntp_snippet... components/ntp_snippets/user_classifier.cc:95: return (event_now ? 1 : 0) + It might be a bit simpler to have the caller add the additional event if necessary. https://codereview.chromium.org/2346263002/diff/100001/components/ntp_snippet... components/ntp_snippets/user_classifier.cc:223: return "Unknown user class"; Just return a std::string, as this is only there to make the compiler happy. If you want to, you can append the message to the NOTREACHED().
Thanks, Bernhard! https://codereview.chromium.org/2346263002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/snippets_internals.html (right): https://codereview.chromium.org/2346263002/diff/100001/chrome/browser/resourc... chrome/browser/resources/snippets_internals.html:67: <input id="clear-classification" type="submit" On 2016/09/20 15:59:54, Bernhard (slow until Sep 27) wrote: > Nit: I think the <button> tag might be a bit more idiomatic for this (using a > submit button technically also gives you a button, but I'm pretty sure dates > back to when HTML was mostly static pages and forms). > > (And yeah, that also applies to the other existing buttons in here. Sorry, I've > missed that previously.) Done all over the file. https://codereview.chromium.org/2346263002/diff/100001/components/ntp_snippet... File components/ntp_snippets/user_classifier.cc (right): https://codereview.chromium.org/2346263002/diff/100001/components/ntp_snippet... components/ntp_snippets/user_classifier.cc:68: static_cast<int>(UserClassifier::Metric::COUNT) && On 2016/09/20 15:59:54, Bernhard (slow until Sep 27) wrote: > Maybe split this up into separate asserts? I think these checks conceptually belong together, if it triggers an error, it is clear how to fix it even though it is just one joint condition. And I do not like the idea of repeating the error literal message 4 times :| Thus I think it is better as it is now. https://codereview.chromium.org/2346263002/diff/100001/components/ntp_snippet... components/ntp_snippets/user_classifier.cc:95: return (event_now ? 1 : 0) + On 2016/09/20 15:59:54, Bernhard (slow until Sep 27) wrote: > It might be a bit simpler to have the caller add the additional event if > necessary. Done. https://codereview.chromium.org/2346263002/diff/100001/components/ntp_snippet... components/ntp_snippets/user_classifier.cc:223: return "Unknown user class"; On 2016/09/20 15:59:54, Bernhard (slow until Sep 27) wrote: > Just return a std::string, as this is only there to make the compiler happy. If > you want to, you can append the message to the NOTREACHED(). Done.
The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by jkrcal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org, bauerb@chromium.org Link to the patchset: https://codereview.chromium.org/2346263002/#ps120001 (title: "Bernhard's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Extending the UserClassifier to actually support classification. This CL adds actual classification to the UserClassifier. So far, only simple classification thresholds are implemented. The CL also includes a new snippets-internals section and a bit of refactoring. In a follow-up CLs, all the parameters will become configurable by variations. BUG=648252 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Extending the UserClassifier to actually support classification. This CL adds actual classification to the UserClassifier. So far, only simple classification thresholds are implemented. The CL also includes a new snippets-internals section and a bit of refactoring. In a follow-up CLs, all the parameters will become configurable by variations. BUG=648252 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/8048f8d8d5153a0e530529e883c78582853d0b15 Cr-Commit-Position: refs/heads/master@{#420022} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/8048f8d8d5153a0e530529e883c78582853d0b15 Cr-Commit-Position: refs/heads/master@{#420022} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
