Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2358983003:
Making UserClassifier configurable by variation parameters (Closed)
|
Created:
4 years, 3 months ago by jkrcal Modified:
4 years, 3 months ago Reviewers:
Marc Treib CC:
chromium-reviews, ntp-dev+reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMaking UserClassifier configurable by variation parameters
Previously, UserClassifier had hard coded constants for computing the
metrics and for classifying users.
After this CL, all the parameters can be specified using variations
service.
BUG=648253
Committed: https://crrev.com/859d31b65c751bdf2fd1a319c63de533493b26b6
Cr-Commit-Position: refs/heads/master@{#420347}
Patch Set 1 #
Total comments: 16
Patch Set 2 : Marc's comments #
Total comments: 6
Patch Set 3 : Marc's comments #2 #
Messages
Total messages: 19 (10 generated)
jkrcal@chromium.org changed reviewers: + treib@chromium.org
Marc, PTAL!
https://codereview.chromium.org/2358983003/diff/1/components/ntp_snippets/use... File components/ntp_snippets/user_classifier.cc (right): https://codereview.chromium.org/2358983003/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier.cc:35: "user_classifier_max_hours"; fits on the previous line https://codereview.chromium.org/2358983003/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier.cc:42: "user_classifier_min_hours"; here too https://codereview.chromium.org/2358983003/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier.cc:82: const char* kDefaultParams[] = { I think these could use a bit more descriptive names. kInitialHoursBetweenEvents[Params] or something? https://codereview.chromium.org/2358983003/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier.cc:94: static_cast<int>(UserClassifier::Metric::COUNT), Add kDefaultParams? https://codereview.chromium.org/2358983003/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier.cc:118: double GetDefaultEstimate(UserClassifier::Metric metric) { GetDefaultEstimateHoursBetweenEvents or GetInitialEstimateHoursBetweenEvents? https://codereview.chromium.org/2358983003/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier.cc:240: kMaxHours, 50); Hm, so the histograms still use kMaxHours rather than max_hours. That's necessary, but maybe worth a comment? https://codereview.chromium.org/2358983003/diff/1/components/ntp_snippets/use... File components/ntp_snippets/user_classifier.h (right): https://codereview.chromium.org/2358983003/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier.h:20: // Based on these lon-term user metrics, it classifies the user in a UserClass. s/lon-term/long-term/ https://codereview.chromium.org/2358983003/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier.h:102: const double active_consumer_opens_ntp_at_least_once_per_hours_; I first wanted to say "s/consumer/user/ to be consistent with the enum", but this actually described the most-active user class ("active consumer"). I find that a bit confusing - any reason for introducing another requirement? Intuitively, the scroll-below-the-fold rate should be sufficient, no?
Description was changed from ========== Making UserClassifier configurable by variation parameters Previously, UserClassifier had hard coded constants for computing the metrics and for classifying users. After this CL, all the parameters can be specified using variations service. Additionally, the CL adds one more parameter for classifying the most active class of users. BUG=648253 ========== to ========== Making UserClassifier configurable by variation parameters Previously, UserClassifier had hard coded constants for computing the metrics and for classifying users. After this CL, all the parameters can be specified using variations service. BUG=648253 ==========
Thanks! PTAL, again. https://codereview.chromium.org/2358983003/diff/1/components/ntp_snippets/use... File components/ntp_snippets/user_classifier.cc (right): https://codereview.chromium.org/2358983003/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier.cc:35: "user_classifier_max_hours"; On 2016/09/22 12:29:05, Marc Treib wrote: > fits on the previous line Done. https://codereview.chromium.org/2358983003/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier.cc:42: "user_classifier_min_hours"; On 2016/09/22 12:29:05, Marc Treib wrote: > here too Done. https://codereview.chromium.org/2358983003/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier.cc:82: const char* kDefaultParams[] = { On 2016/09/22 12:29:05, Marc Treib wrote: > I think these could use a bit more descriptive names. > kInitialHoursBetweenEvents[Params] or something? Done. https://codereview.chromium.org/2358983003/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier.cc:94: static_cast<int>(UserClassifier::Metric::COUNT), On 2016/09/22 12:29:05, Marc Treib wrote: > Add kDefaultParams? Done. https://codereview.chromium.org/2358983003/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier.cc:118: double GetDefaultEstimate(UserClassifier::Metric metric) { On 2016/09/22 12:29:05, Marc Treib wrote: > GetDefaultEstimateHoursBetweenEvents or GetInitialEstimateHoursBetweenEvents? Done. GetInitialHoursBetweenEvents to match the const variables above. Actually there is nothing estimated in the default value (they are just gonna be interpreted as estimates later). https://codereview.chromium.org/2358983003/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier.cc:240: kMaxHours, 50); On 2016/09/22 12:29:05, Marc Treib wrote: > Hm, so the histograms still use kMaxHours rather than max_hours. That's > necessary, but maybe worth a comment? Oh, I missed this, thanks! Comments added. https://codereview.chromium.org/2358983003/diff/1/components/ntp_snippets/use... File components/ntp_snippets/user_classifier.h (right): https://codereview.chromium.org/2358983003/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier.h:20: // Based on these lon-term user metrics, it classifies the user in a UserClass. On 2016/09/22 12:29:06, Marc Treib wrote: > s/lon-term/long-term/ Done. https://codereview.chromium.org/2358983003/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier.h:102: const double active_consumer_opens_ntp_at_least_once_per_hours_; On 2016/09/22 12:29:06, Marc Treib wrote: > I first wanted to say "s/consumer/user/ to be consistent with the enum", but > this actually described the most-active user class ("active consumer"). I find > that a bit confusing - any reason for introducing another requirement? > Intuitively, the scroll-below-the-fold rate should be sufficient, no? Well, I wanted to be on the safe side and have more knobs to control things. You are right, this is unnecessary complexity. When we later see that other ways to classify are absolutely necessary, we might still be able to merge it in M55 Beta. Anyway, classification of active consumers is not that crucial (as classification of rare users).
On 2016/09/22 13:26:15, jkrcal wrote: > Thanks! PTAL, again. > > https://codereview.chromium.org/2358983003/diff/1/components/ntp_snippets/use... > File components/ntp_snippets/user_classifier.cc (right): > > https://codereview.chromium.org/2358983003/diff/1/components/ntp_snippets/use... > components/ntp_snippets/user_classifier.cc:35: "user_classifier_max_hours"; > On 2016/09/22 12:29:05, Marc Treib wrote: > > fits on the previous line > > Done. > > https://codereview.chromium.org/2358983003/diff/1/components/ntp_snippets/use... > components/ntp_snippets/user_classifier.cc:42: "user_classifier_min_hours"; > On 2016/09/22 12:29:05, Marc Treib wrote: > > here too > > Done. > > https://codereview.chromium.org/2358983003/diff/1/components/ntp_snippets/use... > components/ntp_snippets/user_classifier.cc:82: const char* kDefaultParams[] = { > On 2016/09/22 12:29:05, Marc Treib wrote: > > I think these could use a bit more descriptive names. > > kInitialHoursBetweenEvents[Params] or something? > > Done. > > https://codereview.chromium.org/2358983003/diff/1/components/ntp_snippets/use... > components/ntp_snippets/user_classifier.cc:94: > static_cast<int>(UserClassifier::Metric::COUNT), > On 2016/09/22 12:29:05, Marc Treib wrote: > > Add kDefaultParams? > > Done. > > https://codereview.chromium.org/2358983003/diff/1/components/ntp_snippets/use... > components/ntp_snippets/user_classifier.cc:118: double > GetDefaultEstimate(UserClassifier::Metric metric) { > On 2016/09/22 12:29:05, Marc Treib wrote: > > GetDefaultEstimateHoursBetweenEvents or GetInitialEstimateHoursBetweenEvents? > > Done. GetInitialHoursBetweenEvents to match the const variables above. Actually > there is nothing estimated in the default value (they are just gonna be > interpreted as estimates later). > > https://codereview.chromium.org/2358983003/diff/1/components/ntp_snippets/use... > components/ntp_snippets/user_classifier.cc:240: kMaxHours, 50); > On 2016/09/22 12:29:05, Marc Treib wrote: > > Hm, so the histograms still use kMaxHours rather than max_hours. That's > > necessary, but maybe worth a comment? > > Oh, I missed this, thanks! Comments added. > > https://codereview.chromium.org/2358983003/diff/1/components/ntp_snippets/use... > File components/ntp_snippets/user_classifier.h (right): > > https://codereview.chromium.org/2358983003/diff/1/components/ntp_snippets/use... > components/ntp_snippets/user_classifier.h:20: // Based on these lon-term user > metrics, it classifies the user in a UserClass. > On 2016/09/22 12:29:06, Marc Treib wrote: > > s/lon-term/long-term/ > > Done. > > https://codereview.chromium.org/2358983003/diff/1/components/ntp_snippets/use... > components/ntp_snippets/user_classifier.h:102: const double > active_consumer_opens_ntp_at_least_once_per_hours_; > On 2016/09/22 12:29:06, Marc Treib wrote: > > I first wanted to say "s/consumer/user/ to be consistent with the enum", but > > this actually described the most-active user class ("active consumer"). I find > > that a bit confusing - any reason for introducing another requirement? > > Intuitively, the scroll-below-the-fold rate should be sufficient, no? > > Well, I wanted to be on the safe side and have more knobs to control things. You > are right, this is unnecessary complexity. > > When we later see that other ways to classify are absolutely necessary, we might > still be able to merge it in M55 Beta. Anyway, classification of active > consumers is not that crucial (as classification of rare users). By the way, I've also updated go/zine-parameters.
lgtm https://codereview.chromium.org/2358983003/diff/20001/components/ntp_snippets... File components/ntp_snippets/user_classifier.cc (right): https://codereview.chromium.org/2358983003/diff/20001/components/ntp_snippets... components/ntp_snippets/user_classifier.cc:113: discount_factor_per_day = kDiscountFactorPerDay; I guess this should be rare, but maybe add a DLOG or something? https://codereview.chromium.org/2358983003/diff/20001/components/ntp_snippets... components/ntp_snippets/user_classifier.cc:116: return std::log(1.0 / (1.0 - discount_factor_per_day)) / 24.0; Pre-existing: Is there any fundamental difference between "discount rate" and "discount factor"? If not, could you settle on one term? https://codereview.chromium.org/2358983003/diff/20001/components/ntp_snippets... components/ntp_snippets/user_classifier.cc:237: // We use kMaxHours as the max value below as this cannot change on the fly. s/this cannot change on the fly/the maximum value for the histograms must be constant/ or something like that?
Thanks! https://codereview.chromium.org/2358983003/diff/20001/components/ntp_snippets... File components/ntp_snippets/user_classifier.cc (right): https://codereview.chromium.org/2358983003/diff/20001/components/ntp_snippets... components/ntp_snippets/user_classifier.cc:113: discount_factor_per_day = kDiscountFactorPerDay; On 2016/09/22 13:36:09, Marc Treib wrote: > I guess this should be rare, but maybe add a DLOG or something? Done. https://codereview.chromium.org/2358983003/diff/20001/components/ntp_snippets... components/ntp_snippets/user_classifier.cc:116: return std::log(1.0 / (1.0 - discount_factor_per_day)) / 24.0; On 2016/09/22 13:36:09, Marc Treib wrote: > Pre-existing: Is there any fundamental difference between "discount rate" and > "discount factor"? If not, could you settle on one term? The notion discount factor is usually used in discrete discounting whereas rate in continuous discounting. By the notion "factor" I wanted to make it easier to understand as something "simple and discrete":) Probably more misleading than helpful -> changed to rate for both cases. https://codereview.chromium.org/2358983003/diff/20001/components/ntp_snippets... components/ntp_snippets/user_classifier.cc:237: // We use kMaxHours as the max value below as this cannot change on the fly. On 2016/09/22 13:36:09, Marc Treib wrote: > s/this cannot change on the fly/the maximum value for the histograms must be > constant/ or something like that? 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 Link to the patchset: https://codereview.chromium.org/2358983003/#ps40001 (title: "Marc's comments #2")
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.
Description was changed from ========== Making UserClassifier configurable by variation parameters Previously, UserClassifier had hard coded constants for computing the metrics and for classifying users. After this CL, all the parameters can be specified using variations service. BUG=648253 ========== to ========== Making UserClassifier configurable by variation parameters Previously, UserClassifier had hard coded constants for computing the metrics and for classifying users. After this CL, all the parameters can be specified using variations service. BUG=648253 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Making UserClassifier configurable by variation parameters Previously, UserClassifier had hard coded constants for computing the metrics and for classifying users. After this CL, all the parameters can be specified using variations service. BUG=648253 ========== to ========== Making UserClassifier configurable by variation parameters Previously, UserClassifier had hard coded constants for computing the metrics and for classifying users. After this CL, all the parameters can be specified using variations service. BUG=648253 Committed: https://crrev.com/859d31b65c751bdf2fd1a319c63de533493b26b6 Cr-Commit-Position: refs/heads/master@{#420347} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/859d31b65c751bdf2fd1a319c63de533493b26b6 Cr-Commit-Position: refs/heads/master@{#420347} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
