Extending the UserClassifier to actually support classification.

components/ntp_snippets/user_classifier.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/ntp_snippets/user_classifier.h"
 
 #include <float.h>
 
 #include <algorithm>
 #include <string>
 
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/string_number_conversions.h"
 #include "components/ntp_snippets/pref_names.h"
 #include "components/prefs/pref_registry_simple.h"
 #include "components/prefs/pref_service.h"
 
+namespace ntp_snippets {
+
 namespace {
 
 // TODO(jkrcal): Make all of this configurable via variations_service.
 
 // The discount factor for computing the discounted-average metrics. Must be
 // strictly larger than 0 and strictly smaller than 1!
 const double kDiscountFactorPerDay = 0.25;
 
 // Never consider any larger interval than this (so that extreme situations such
 // as losing your phone or going for a long offline vacation do not skew the
 // average too much).
 const double kMaxHours = 7 * 24;
 
 // Ignore events within |kMinHours| hours since the last event (|kMinHours| is
 // the length of the browsing session where subsequent events of the same type
 // do not count again).
 const double kMinHours = 0.5;
 
+// Classification constants.
+const double kFrequentUserScrollsAtLeastOncePerHours = 24;
+const double kOccasionalUserOpensNTPAtMostOncePerHours = 72;
+
 const char kHistogramAverageHoursToOpenNTP[] =
     "NewTabPage.UserClassifier.AverageHoursToOpenNTP";
 const char kHistogramAverageHoursToShowSuggestions[] =
     "NewTabPage.UserClassifier.AverageHoursToShowSuggestions";
 const char kHistogramAverageHoursToUseSuggestions[] =
     "NewTabPage.UserClassifier.AverageHoursToUseSuggestions";
 
-}  // namespace
-
-namespace ntp_snippets {
-
-UserClassifier::UserClassifier(PrefService* pref_service)
-    : pref_service_(pref_service),
-      // Compute discount_rate_per_hour such that
-      //   kDiscountFactorPerDay = 1 - e^{-discount_rate_per_hour * 24}.
-      discount_rate_per_hour_(std::log(1 / (1 - kDiscountFactorPerDay)) / 24) {}
-
-UserClassifier::~UserClassifier() {}
-
-// static
-void UserClassifier::RegisterProfilePrefs(PrefRegistrySimple* registry) {
-  registry->RegisterDoublePref(
-      prefs::kUserClassifierAverageNTPOpenedPerHour, 1);
-  registry->RegisterDoublePref(
-      prefs::kUserClassifierAverageSuggestionsShownPerHour, 1);
-  registry->RegisterDoublePref(
-      prefs::kUserClassifierAverageSuggestionsUsedPerHour, 1);
-
-  registry->RegisterInt64Pref(prefs::kUserClassifierLastTimeToOpenNTP, 0);
-  registry->RegisterInt64Pref(prefs::kUserClassifierLastTimeToShowSuggestions,
-                              0);
-  registry->RegisterInt64Pref(prefs::kUserClassifierLastTimeToUseSuggestions,
-                              0);
-}
-
-void UserClassifier::OnNTPOpened() {
-  UpdateMetricOnEvent(prefs::kUserClassifierAverageNTPOpenedPerHour,
-                      prefs::kUserClassifierLastTimeToOpenNTP);
-
-  double avg = GetEstimateHoursBetweenEvents(
-      prefs::kUserClassifierAverageNTPOpenedPerHour);
-  UMA_HISTOGRAM_CUSTOM_COUNTS(kHistogramAverageHoursToOpenNTP, avg, 1,
-                              kMaxHours, 50);
-}
-
-void UserClassifier::OnSuggestionsShown() {
-  UpdateMetricOnEvent(prefs::kUserClassifierAverageSuggestionsShownPerHour,
-                      prefs::kUserClassifierLastTimeToShowSuggestions);
-
-  double avg = GetEstimateHoursBetweenEvents(
-      prefs::kUserClassifierAverageSuggestionsShownPerHour);
-  UMA_HISTOGRAM_CUSTOM_COUNTS(kHistogramAverageHoursToShowSuggestions, avg, 1,
-                              kMaxHours, 50);
-}
-
-void UserClassifier::OnSuggestionsUsed() {
-  UpdateMetricOnEvent(prefs::kUserClassifierAverageSuggestionsUsedPerHour,
-                      prefs::kUserClassifierLastTimeToUseSuggestions);
-
-  double avg = GetEstimateHoursBetweenEvents(
-      prefs::kUserClassifierAverageSuggestionsUsedPerHour);
-  UMA_HISTOGRAM_CUSTOM_COUNTS(kHistogramAverageHoursToUseSuggestions, avg, 1,
-                              kMaxHours, 50);
-}
-
-void UserClassifier::UpdateMetricOnEvent(const char* metric_pref_name,
-                                         const char* last_time_pref_name) {
-  if (!pref_service_)
-    return;
-
-  double hours_since_last_time =
-      std::min(kMaxHours, GetHoursSinceLastTime(last_time_pref_name));
-  // Ignore events within the same "browsing session".
-  if (hours_since_last_time < kMinHours)
-    return;
-  SetLastTimeToNow(last_time_pref_name);
-
-  double avg_events_per_hour = pref_service_->GetDouble(metric_pref_name);
+// The summary of the prefs.
+const char* kMetricKeys[] = {
+    prefs::kUserClassifierAverageNTPOpenedPerHour,
+    prefs::kUserClassifierAverageSuggestionsShownPerHour,
+    prefs::kUserClassifierAverageSuggestionsUsedPerHour};
+const char* kLastTimeKeys[] = {prefs::kUserClassifierLastTimeToOpenNTP,
+                               prefs::kUserClassifierLastTimeToShowSuggestions,
+                               prefs::kUserClassifierLastTimeToUseSuggestions};
+
+// Default lengths of the intervals for new users for the metrics.
+const double kDefaults[] = {24, 36, 48};

[Marc Treib, 2016/09/20 10:27:03]

optional: add some static_asserts to make sure these all have the correct size?
(Probably by adding a Metric::COUNT)

[jkrcal, 2016/09/20 13:10:13]

Done.

+
+// Computes the discount rate.
+double
+GetDiscountRatePerHour() {

[Marc Treib, 2016/09/20 10:27:03]

extra line break

[jkrcal, 2016/09/20 13:10:13]

Done.

+  static double discount_rate_per_hour = 0.0;
+
+  if (discount_rate_per_hour == 0.0) {

[tschumann, 2016/09/20 11:32:47]

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.

[jkrcal, 2016/09/20 13:10:13]

It was initialization, I removed the static var, after Marc's comments.

+    // Compute discount_rate_per_hour such that
+    //   kDiscountFactorPerDay = 1 - e^{-discount_rate_per_hour * 24}.
+    discount_rate_per_hour =
+        std::log(1.0 / (1.0 - kDiscountFactorPerDay)) / 24.0;
+  }
+
+  return discount_rate_per_hour;
+}
+
+// Returns the new value of the metric using its |old_value|, assuming
+// |hours_since_last_time| hours have passed since it was last recomputed.
+// If |event_now| is true, the event is assumed to have happened right now,
+// otherwise no event is assumed to happen within the last
+// |hours_since_last_time| hours.
+double RecomputeMetric(double old_value,
+                       double hours_since_last_time,
+                       bool event_now) {
   // Compute and store the new discounted average according to the formula
   //   avg_events := 1 + e^{-discount_rate_per_hour * hours_since} * avg_events.
-  double new_avg_events_per_hour =
-      1 +
-      std::exp(-discount_rate_per_hour_ * hours_since_last_time) *
-          avg_events_per_hour;
-  pref_service_->SetDouble(metric_pref_name, new_avg_events_per_hour);
-}
-
-double UserClassifier::GetEstimateHoursBetweenEvents(
-    const char* metric_pref_name) {
-  double avg_events_per_hour = pref_service_->GetDouble(metric_pref_name);
-
+  return (event_now ? 1 : 0) +
+         std::exp(-GetDiscountRatePerHour() * hours_since_last_time) *
+             old_value;
+}
+
+// Compute the number of hours between two events for the given metric value
+// assuming the events were equally distributed.
+double GetEstimateHoursBetweenEvents(const double metric_value) {
   // Right after the first update, the metric is equal to 1.
-  if (avg_events_per_hour <= 1)
+  if (metric_value <= 1)

[tschumann, 2016/09/20 11:32:47]

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.

[jkrcal, 2016/09/20 13:10:13]

Done.

     return kMaxHours;
 
   // This is the estimate with the assumption that last event happened right
   // now and the system is in the steady-state. Solve estimate_hours in the
   // steady-state equation:
-  //   avg_events = 1 + e^{-discount_rate * estimate_hours} * avg_events,
+  //   metric_value = 1 + e^{-discount_rate * estimate_hours} * metric_value,
   // i.e.
-  //   -discount_rate * estimate_hours = log((avg_events - 1) / avg_events),
-  //   discount_rate * estimate_hours = log(avg_events / (avg_events - 1)),
-  //   estimate_hours = log(avg_events / (avg_events - 1)) / discount_rate.
-  return std::min(kMaxHours,
-                  std::log(avg_events_per_hour / (avg_events_per_hour - 1)) /
-                      discount_rate_per_hour_);
-}
-
-double UserClassifier::GetHoursSinceLastTime(
-    const char* last_time_pref_name) {
-  if (!pref_service_->HasPrefPath(last_time_pref_name))
-    return DBL_MAX;
+  //   -discount_rate * estimate_hours = log((metric_value - 1) / metric_value),
+  //   discount_rate * estimate_hours = log(metric_value / (metric_value - 1)),
+  //   estimate_hours = log(metric_value / (metric_value - 1)) / discount_rate.
+  double estimate_hours =
+      std::log(metric_value / (metric_value - 1)) / GetDiscountRatePerHour();
+  return std::max(kMinHours, std::min(kMaxHours, estimate_hours));
+}
+
+// The inverse of GetEstimateHoursBetweenEvents().
+double GetMetricValueForEstimateHoursBetweenEvents(double estimate_hours) {
+  // Keep the input value within [kMinHours, kMaxHours].
+  estimate_hours = std::max(kMinHours, std::min(kMaxHours, estimate_hours));
+
+  // Return |metric_value| such that GetEstimateHoursBetweenEvents for
+  // |metric_value| returns |estimate_hours|. Thus, solve |metric_value| in
+  //   metric_value = 1 + e^{-discount_rate * estimate_hours} * metric_value,
+  // i.e.
+  //   metric_value * (1 - e^{-discount_rate * estimate_hours}) = 1,
+  //   metric_value = 1 / (1 - e^{-discount_rate * estimate_hours}).
+  return 1.0 / (1.0 - std::exp(-GetDiscountRatePerHour() * estimate_hours));
+}
+
+}  // namespace
+
+// static
+const UserClassifier::Metric UserClassifier::kMetrics[3] = {

[Marc Treib, 2016/09/20 10:27:03]

Is the "3" required?

[jkrcal, 2016/09/20 13:10:13]

Done.

+    Metric::OPEN_NTP, Metric::SHOW_SUGGESTIONS, Metric::USE_SUGGESTIONS};
+
+UserClassifier::UserClassifier(PrefService* pref_service)
+    : pref_service_(pref_service) {
+  // The pref_service_ can be null in tests.
+  if (!pref_service_)
+    return;
+
+  // Initialize the prefs storing the last time: the counter has just started!
+  for (const Metric metric : kMetrics) {
+    if (!HasLastTime(metric))
+      SetLastTimeToNow(metric);
+  }
+}
+
+UserClassifier::~UserClassifier() {}
+
+// static
+void UserClassifier::RegisterProfilePrefs(PrefRegistrySimple* registry) {
+  for (const Metric metric : kMetrics) {
+    registry->RegisterDoublePref(kMetricKeys[metric],
+                                 GetMetricValueForEstimateHoursBetweenEvents(
+                                     kDefaults[metric]));
+    registry->RegisterInt64Pref(kLastTimeKeys[metric], 0);
+  }
+}
+
+void UserClassifier::OnNTPOpened() {
+  double metric = UpdateMetricOnEvent(Metric::OPEN_NTP);
+
+  double avg = GetEstimateHoursBetweenEvents(metric);
+  UMA_HISTOGRAM_CUSTOM_COUNTS(kHistogramAverageHoursToOpenNTP, avg, 1,
+                              kMaxHours, 50);
+}
+
+void UserClassifier::OnSuggestionsShown() {
+  double metric = UpdateMetricOnEvent(Metric::SHOW_SUGGESTIONS);
+
+  double avg = GetEstimateHoursBetweenEvents(metric);
+  UMA_HISTOGRAM_CUSTOM_COUNTS(kHistogramAverageHoursToShowSuggestions, avg, 1,
+                              kMaxHours, 50);
+}
+
+void UserClassifier::OnSuggestionsUsed() {
+  double metric = UpdateMetricOnEvent(Metric::USE_SUGGESTIONS);
+
+  double avg = GetEstimateHoursBetweenEvents(metric);
+  UMA_HISTOGRAM_CUSTOM_COUNTS(kHistogramAverageHoursToUseSuggestions, avg, 1,
+                              kMaxHours, 50);
+}
+
+double UserClassifier::GetEstimatedAvgTimeToOpenNTP() const {

[Marc Treib, 2016/09/20 10:27:03]

Are the three GetEstimatedAvgTimeTo... methods still useful? Looks like we might
just as well call GetEstimatedAvgTime(Metric::...)

[jkrcal, 2016/09/20 13:10:13]

Done.

+  return GetEstimatedAvgTime(Metric::OPEN_NTP);
+}
+
+double UserClassifier::GetEstimatedAvgTimeToShowSuggestions() const {
+  return GetEstimatedAvgTime(Metric::SHOW_SUGGESTIONS);
+}
+
+double UserClassifier::GetEstimatedAvgTimeToUseSuggestions() const {
+  return GetEstimatedAvgTime(Metric::USE_SUGGESTIONS);
+}
+
+UserClassifier::UserClass UserClassifier::GetUserClass() const {
+  if (GetEstimatedAvgTimeToOpenNTP() >=
+      kOccasionalUserOpensNTPAtMostOncePerHours) {
+    return UserClass::OCCASIONAL_NTP_USER;
+  }
+
+  if (GetEstimatedAvgTimeToUseSuggestions() <=
+      kFrequentUserScrollsAtLeastOncePerHours) {
+    return UserClass::FREQUENT_NTP_USER;
+  }
+
+  return UserClass::NORMAL_NTP_USER;
+}
+
+std::string UserClassifier::GetUserClassDescriptionForDebugging() const {
+  switch (GetUserClass()) {
+    case UserClass::OCCASIONAL_NTP_USER:
+      return "Occasional user of the NTP";
+    case UserClass::NORMAL_NTP_USER:
+      return "Normal user of the NTP";
+    case UserClass::FREQUENT_NTP_USER:
+      return "Frequent user of the NTP";
+  }
+  NOTREACHED();
+  return "Unknown user class";
+}
+
+void UserClassifier::ClearClassificationForDebugging() {
+  // The pref_service_ can be null in tests.
+  if (!pref_service_)
+    return;
+
+  for (const Metric& metric : kMetrics) {
+    ClearMetricValue(metric);
+    SetLastTimeToNow(metric);
+  }
+}
+
+double UserClassifier::GetEstimatedAvgTime(Metric metric) const {
+  double metric_value = GetUpToDateMetricValue(metric);
+  return GetEstimateHoursBetweenEvents(metric_value);
+}
+
+double UserClassifier::UpdateMetricOnEvent(Metric metric) {
+  // The pref_service_ can be null in tests.
+  if (!pref_service_)
+    return 0;
+
+  double hours_since_last_time =
+      std::min(kMaxHours, GetHoursSinceLastTime(metric));
+  // Ignore events within the same "browsing session".
+  if (hours_since_last_time < kMinHours)
+    return GetUpToDateMetricValue(metric);
+
+  SetLastTimeToNow(metric);
+
+  double metric_value = GetMetricValue(metric);
+  double new_metric_value = RecomputeMetric(metric_value, hours_since_last_time,
+                                            true /* event_now */);

[Marc Treib, 2016/09/20 10:27:03]

nit: Preferred style is /*event_now=*/true

[jkrcal, 2016/09/20 13:10:13]

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 :)

[Marc Treib, 2016/09/20 13:26:52]

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. :)

[jkrcal, 2016/09/20 13:46:39]

Done.

[Bernhard Bauer, 2016/09/20 15:59:54]

Weeeelll… If that tool needs to be written / ported for Chromium anyway, we
might be able to teach it both styles :)

+  SetMetricValue(metric, new_metric_value);
+  return new_metric_value;
+}
+
+double UserClassifier::GetUpToDateMetricValue(Metric metric) const {
+  // The pref_service_ can be null in tests.
+  if (!pref_service_)
+    return 0;
+
+  double hours_since_last_time =
+      std::min(kMaxHours, GetHoursSinceLastTime(metric));
+
+  double metric_value = GetMetricValue(metric);
+  return RecomputeMetric(metric_value, hours_since_last_time,
+                         false /* event_now */);
+}
+
+double UserClassifier::GetHoursSinceLastTime(Metric metric) const {
+  DCHECK(pref_service_);
+  if (!HasLastTime(metric))
+    return 0;
 
   base::TimeDelta since_last_time =
       base::Time::Now() - base::Time::FromInternalValue(
-                              pref_service_->GetInt64(last_time_pref_name));
+                              pref_service_->GetInt64(kLastTimeKeys[metric]));
   return since_last_time.InSecondsF() / 3600;
 }
 
-void UserClassifier::SetLastTimeToNow(const char* last_time_pref_name) {
-  pref_service_->SetInt64(last_time_pref_name,
+bool UserClassifier::HasLastTime(const Metric metric) const {
+  DCHECK(pref_service_);

[Marc Treib, 2016/09/20 10:27:03]

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.

[jkrcal, 2016/09/20 13:10:13]

Done.

+  return pref_service_->HasPrefPath(kLastTimeKeys[metric]);
+}
+
+void UserClassifier::SetLastTimeToNow(Metric metric) {
+  DCHECK(pref_service_);
+  pref_service_->SetInt64(kLastTimeKeys[metric],
                           base::Time::Now().ToInternalValue());
 }
 
+double UserClassifier::GetMetricValue(const Metric metric) const {
+  DCHECK(pref_service_);
+  return pref_service_->GetDouble(kMetricKeys[metric]);
+}
+
+void UserClassifier::SetMetricValue(const Metric metric, double metric_value) {
+  DCHECK(pref_service_);
+  pref_service_->SetDouble(kMetricKeys[metric], metric_value);
+}
+
+void UserClassifier::ClearMetricValue(const Metric metric) {
+  DCHECK(pref_service_);
+  pref_service_->ClearPref(kMetricKeys[metric]);
+}
+
 }  // namespace ntp_snippets