Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2732243002:
[UserClassifier] Fix the classification for M58 condensed NTP experiment (Closed)
Description[UserClassifier] Fix the classification for M58 condensed NTP experiment
Previously, UserClassifier used suggestions impressions as activity
signal. This breaks for condensed NTP when impressions are tied to
opening an NTP.
This CL replaces this signal by clicks. Clicks were previously recorded
and not used. After this CL, impressions will stay being recorded. The
reason is that with further changes in the UI, we may need it again. There is a TODO to remove this metric if not used.
This CL also changes default values for activity metrics. The reason is
to make sure that a new user is classified in the middle class.
Lastly, the CL relaxes the thresholds. Currently, we have 67% rare, 33% active users, 1% active consumers. The aim is to move closer towards 55% / 40% / 5%.
BUG=699021
Review-Url: https://codereview.chromium.org/2732243002
Cr-Commit-Position: refs/heads/master@{#455734}
Committed: https://chromium.googlesource.com/chromium/src/+/f215f20a3eba699ac39d8bc5c0e4cf8e0d9b0610
Patch Set 1 #
Total comments: 4
Patch Set 2 : Comments #
Messages
Total messages: 24 (15 generated)
Description was changed from ========== [UserClassifier] Fix the classification for M58 condensed NTP experiment Previously, UserClassifier used suggestions impressions as activity signal. This breaks for condensed NTP when impressions are tied to opening an NTP. This CL replaces this signal by clicks. Clicks were previously recorded and not used. After this CL, impressions will stay being recorded. The reason is that with further changes in the UI, we may need it again soon. BUG=699021 ========== to ========== [UserClassifier] Fix the classification for M58 condensed NTP experiment Previously, UserClassifier used suggestions impressions as activity signal. This breaks for condensed NTP when impressions are tied to opening an NTP. This CL replaces this signal by clicks. Clicks were previously recorded and not used. After this CL, impressions will stay being recorded. The reason is that with further changes in the UI, we may need it again soon. This CL also changes default values for activity metrics. The reason is to make sure that a new user is classified in the middle class. BUG=699021 ==========
jkrcal@chromium.org changed reviewers: + tschumann@chromium.org
Tim, could you PTAL?
lgtm Can you add here or to the bug what the expected change in size of the active-user bucket will be?
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...
Description was changed from ========== [UserClassifier] Fix the classification for M58 condensed NTP experiment Previously, UserClassifier used suggestions impressions as activity signal. This breaks for condensed NTP when impressions are tied to opening an NTP. This CL replaces this signal by clicks. Clicks were previously recorded and not used. After this CL, impressions will stay being recorded. The reason is that with further changes in the UI, we may need it again soon. This CL also changes default values for activity metrics. The reason is to make sure that a new user is classified in the middle class. BUG=699021 ========== to ========== [UserClassifier] Fix the classification for M58 condensed NTP experiment Previously, UserClassifier used suggestions impressions as activity signal. This breaks for condensed NTP when impressions are tied to opening an NTP. This CL replaces this signal by clicks. Clicks were previously recorded and not used. After this CL, impressions will stay being recorded. The reason is that with further changes in the UI, we may need it again soon. This CL also changes default values for activity metrics. The reason is to make sure that a new user is classified in the middle class. Lastly, the CL relaxes the thresholds. Currently, we have 67% rare, 33% active users, 1% active consumers. The aim is to move closer towards 55% / 40% / 5%. BUG=699021 ==========
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
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
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
jkrcal@chromium.org changed reviewers: + treib@chromium.org
Huh, Tim is still not a commiter. Marc, could you PTAL? (this is a merge candidate)
lgtm I have it on good authority that Tim's non-committer status will change fairly soon ;) https://codereview.chromium.org/2732243002/diff/1/components/ntp_snippets/use... File components/ntp_snippets/user_classifier.cc (left): https://codereview.chromium.org/2732243002/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier.cc:46: "user_classifier_active_consumer_scrolls_at_least_once_per_hours"; I assume we never had any experiments that set this param? https://codereview.chromium.org/2732243002/diff/1/components/ntp_snippets/use... File components/ntp_snippets/user_classifier.h (right): https://codereview.chromium.org/2732243002/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier.h:44: SUGGESTIONS_SHOWN, // When the content suggestions are shown to the user - Is this metric still useful at all? If not, since this is urgent, just add a TODO to remove it at some point (and to warn people not to rely on it).
Description was changed from ========== [UserClassifier] Fix the classification for M58 condensed NTP experiment Previously, UserClassifier used suggestions impressions as activity signal. This breaks for condensed NTP when impressions are tied to opening an NTP. This CL replaces this signal by clicks. Clicks were previously recorded and not used. After this CL, impressions will stay being recorded. The reason is that with further changes in the UI, we may need it again soon. This CL also changes default values for activity metrics. The reason is to make sure that a new user is classified in the middle class. Lastly, the CL relaxes the thresholds. Currently, we have 67% rare, 33% active users, 1% active consumers. The aim is to move closer towards 55% / 40% / 5%. BUG=699021 ========== to ========== [UserClassifier] Fix the classification for M58 condensed NTP experiment Previously, UserClassifier used suggestions impressions as activity signal. This breaks for condensed NTP when impressions are tied to opening an NTP. This CL replaces this signal by clicks. Clicks were previously recorded and not used. After this CL, impressions will stay being recorded. The reason is that with further changes in the UI, we may need it again. There is a TODO to remove this metric if not used. This CL also changes default values for activity metrics. The reason is to make sure that a new user is classified in the middle class. Lastly, the CL relaxes the thresholds. Currently, we have 67% rare, 33% active users, 1% active consumers. The aim is to move closer towards 55% / 40% / 5%. BUG=699021 ==========
Thanks! https://codereview.chromium.org/2732243002/diff/1/components/ntp_snippets/use... File components/ntp_snippets/user_classifier.cc (left): https://codereview.chromium.org/2732243002/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier.cc:46: "user_classifier_active_consumer_scrolls_at_least_once_per_hours"; On 2017/03/09 10:14:35, Marc Treib wrote: > I assume we never had any experiments that set this param? Correct. I updated the table in go/zine-parameters. https://codereview.chromium.org/2732243002/diff/1/components/ntp_snippets/use... File components/ntp_snippets/user_classifier.h (right): https://codereview.chromium.org/2732243002/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier.h:44: SUGGESTIONS_SHOWN, // When the content suggestions are shown to the user - On 2017/03/09 10:14:35, Marc Treib wrote: > Is this metric still useful at all? If not, since this is urgent, just add a > TODO to remove it at some point (and to warn people not to rely on it). Added the todo.
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, tschumann@chromium.org Link to the patchset: https://codereview.chromium.org/2732243002/#ps20001 (title: "Comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1489062832363580,
"parent_rev": "31069226a11b4a51b02b794fe6ca04342703a069", "commit_rev":
"f215f20a3eba699ac39d8bc5c0e4cf8e0d9b0610"}
Message was sent while issue was closed.
Description was changed from ========== [UserClassifier] Fix the classification for M58 condensed NTP experiment Previously, UserClassifier used suggestions impressions as activity signal. This breaks for condensed NTP when impressions are tied to opening an NTP. This CL replaces this signal by clicks. Clicks were previously recorded and not used. After this CL, impressions will stay being recorded. The reason is that with further changes in the UI, we may need it again. There is a TODO to remove this metric if not used. This CL also changes default values for activity metrics. The reason is to make sure that a new user is classified in the middle class. Lastly, the CL relaxes the thresholds. Currently, we have 67% rare, 33% active users, 1% active consumers. The aim is to move closer towards 55% / 40% / 5%. BUG=699021 ========== to ========== [UserClassifier] Fix the classification for M58 condensed NTP experiment Previously, UserClassifier used suggestions impressions as activity signal. This breaks for condensed NTP when impressions are tied to opening an NTP. This CL replaces this signal by clicks. Clicks were previously recorded and not used. After this CL, impressions will stay being recorded. The reason is that with further changes in the UI, we may need it again. There is a TODO to remove this metric if not used. This CL also changes default values for activity metrics. The reason is to make sure that a new user is classified in the middle class. Lastly, the CL relaxes the thresholds. Currently, we have 67% rare, 33% active users, 1% active consumers. The aim is to move closer towards 55% / 40% / 5%. BUG=699021 Review-Url: https://codereview.chromium.org/2732243002 Cr-Commit-Position: refs/heads/master@{#455734} Committed: https://chromium.googlesource.com/chromium/src/+/f215f20a3eba699ac39d8bc5c0e4... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/f215f20a3eba699ac39d8bc5c0e4... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
