Extending the UserClassifier to actually support classification.

components/ntp_snippets/user_classifier.h

Index: components/ntp_snippets/user_classifier.h
diff --git a/components/ntp_snippets/user_classifier.h b/components/ntp_snippets/user_classifier.h
index 3314884606f42b125937584ed468d285b0c3ad1d..95b4727f1b5855a39258963f2839f06e46ba076c 100644
--- a/components/ntp_snippets/user_classifier.h
+++ b/components/ntp_snippets/user_classifier.h
@@ -5,7 +5,10 @@
 #ifndef COMPONENTS_NTP_SNIPPETS_USER_CLASSIFIER_H_
 #define COMPONENTS_NTP_SNIPPETS_USER_CLASSIFIER_H_
 
+#include <string>
+
 #include "base/macros.h"
+#include "base/time/time.h"
 
 class PrefRegistrySimple;
 class PrefService;
@@ -14,10 +17,16 @@ namespace ntp_snippets {
 
 // Collects data about user usage patterns of content suggestions, computes
 // long-term user metrics locally using pref, and reports the metrics to UMA.
-// TODO(jkrcal): Add classification of users based on the metrics and getters
-// for the classification as well as for the metrics.
+// Based on these lon-term user metrics, it classifies the user in a UserClass.
 class UserClassifier {
  public:
+  // Enumeration listing user classes
+  enum class UserClass {
+    OCCASIONAL_NTP_USER,

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

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

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

Done - RARE.
Replace NORMAL by ORDINARY. Have really spent some time thinking, could not come
up with anything better. WDYT?

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

MODERATE?

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

Renamed differently after an offline discussion with Tim.

+    NORMAL_NTP_USER,
+    FREQUENT_NTP_USER,
+  };
+
   // The provided |pref_service| may be nullptr in unit-tests.
   explicit UserClassifier(PrefService* pref_service);
   ~UserClassifier();
@@ -36,22 +45,55 @@ class UserClassifier {
   // When the user clicks on some suggestions or on some "More" button.
   void OnSuggestionsUsed();
 
- private:
-  // The event has happened, recompute and store the metric accordingly.
-  void UpdateMetricOnEvent(const char* metric_pref_name,
-                           const char* last_time_pref_name);
+  // Get the estimate average length of the interval between two successive
+  // events of the given type.
+  double GetEstimatedAvgTimeToOpenNTP() const;
+  double GetEstimatedAvgTimeToShowSuggestions() const;
+  double GetEstimatedAvgTimeToUseSuggestions() const;
 
-  // Compute the number of hours between two events for the given metric value
-  // assuming the events were equally distributed.
-  double GetEstimateHoursBetweenEvents(const char* metric_pref_name);
+  // Return the classification of the current user.
+  UserClass GetUserClass() const;
+  std::string GetUserClassDescriptionForDebugging() const;
 
-  // Returns the number of hours since the last event of the same type or
-  // DBL_MAX if there is no last event of that type.
-  double GetHoursSinceLastTime(const char* last_time_pref_name);
-  void SetLastTimeToNow(const char* last_time_pref_name);
+  // Resets the classification (emulates a fresh upgrade / install).
+  void ClearClassificationForDebugging();
+
+ private:
+  // For estimating the average length of the intervals between two successive
+  // event, we keep a simple frequency model, a single value that we call
+  // "metric" below.
+  // We track exponentially-discounted rate of the given event per hour where
+  // the continuous utility function between two successive events (e.g. opening
+  // a NTP) at times t1 < t2 is 1 / (t2-t1), i.e. intuitively the rate of this
+  // event in this time interval.
+  // See https://en.wikipedia.org/wiki/Exponential_discounting for more details.
+
+  enum Metric { OPEN_NTP = 0, SHOW_SUGGESTIONS, USE_SUGGESTIONS };

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

enum class? You're using it like "Metric::..." anyway.

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

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.

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

Tim, done.

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

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?

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

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.

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

Done.

+  static const Metric kMetrics[3];

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

Could you just define this in the .cc?

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

Done. (I needed to make the Metric enum public which was anyway useful for other
changes you proposed.)

+
+  // Get the estimate average length of the interval between two successive
+  // events of the given type.
+  double GetEstimatedAvgTime(const Metric metric) const;

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

nit: "const Metric" doesn't do anything here - you're passing by value anyway.

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

Done.

+
+  // The event has happened, recompute the metric accordingly. Then store and
+  // return the new value.
+  double UpdateMetricOnEvent(const Metric metric);
+  // No event has happened but we need to get up-to-date metric, recompute and
+  // return the new value. This function does not store the recomputed metric.
+  double GetUpToDateMetricValue(const Metric metric) const;
+
+  // Returns the number of hours since the last event of the same type.
+  // If there is no last event of that type, assume it happened just now and
+  // return 0.
+  double GetHoursSinceLastTime(const Metric metric) const;
+  bool HasLastTime(const Metric metric) const;
+  void SetLastTimeToNow(const Metric metric);
+
+  double GetMetricValue(const Metric metric) const;
+  void SetMetricValue(const Metric metric, double metric_value);
+  void ClearMetricValue(const Metric metric);
 
   PrefService* pref_service_;
-  double const discount_rate_per_hour_;
 
   DISALLOW_COPY_AND_ASSIGN(UserClassifier);
 };