Adding knobs on HQP provider.

chrome/browser/history/scored_history_match.cc

 // Copyright (c) 2012 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 "chrome/browser/history/scored_history_match.h"
 
 #include <algorithm>
 #include <functional>
 #include <iterator>
 #include <numeric>
 #include <set>
 
 #include <math.h>
 
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/autocomplete/history_url_provider.h"
 #include "components/bookmarks/browser/bookmark_utils.h"
 #include "components/history/core/browser/history_client.h"
 #include "components/omnibox/omnibox_field_trial.h"
 #include "components/omnibox/url_prefix.h"
 #include "content/public/browser/browser_thread.h"
 
 namespace history {
 
 // ScoredHistoryMatch ----------------------------------------------------------
 
 // static
 const size_t ScoredHistoryMatch::kMaxVisitsToScore = 10;
 const int ScoredHistoryMatch::kDaysToPrecomputeRecencyScoresFor = 366;
 const int ScoredHistoryMatch::kMaxRawTermScore = 30;
 float* ScoredHistoryMatch::raw_term_score_to_topicality_score_ = NULL;
 float* ScoredHistoryMatch::days_ago_to_recency_score_ = NULL;
 bool ScoredHistoryMatch::initialized_ = false;
 int ScoredHistoryMatch::bookmark_value_ = 1;
 bool ScoredHistoryMatch::allow_tld_matches_ = false;
 bool ScoredHistoryMatch::allow_scheme_matches_ = false;
 bool ScoredHistoryMatch::also_do_hup_like_scoring_ = false;
 int ScoredHistoryMatch::max_assigned_score_for_non_inlineable_matches_ = -1;
 
+bool ScoredHistoryMatch::hqp_experimental_scoring_enabled_ = false;

[Mark P, 2015/02/14 01:27:14]

(1) These static initializers should be in the order of their declaration in the
file.
(Respond to this after you change the header.)
(2) There's no reason to separate these variables from the others.

[Ashok vardhan, 2015/02/17 01:23:52]

Done.

+float ScoredHistoryMatch::topicality_threshold_ = -1;
+std::vector<ScoredHistoryMatch::ScoreMaxRelevance>*
+    ScoredHistoryMatch::hqp_relevance_buckets_ = NULL;
+
 ScoredHistoryMatch::ScoredHistoryMatch()
     : raw_score_(0),
       can_inline_(false) {
   Init();
 }
 
 ScoredHistoryMatch::ScoredHistoryMatch(
     const URLRow& row,
     const VisitInfoVector& visits,
     const std::string& languages,
@@ skipping 94 matching lines @@
     const int num_components_in_best_inlineable_prefix =
         best_inlineable_prefix->num_components;
     innermost_match = (num_components_in_best_inlineable_prefix ==
         num_components_in_best_prefix);
   }
 
   const float topicality_score = GetTopicalityScore(
       terms.size(), url, terms_to_word_starts_offsets, word_starts);
   const float frequency_score = GetFrequency(
       now, (history_client && history_client->IsBookmarked(gurl)), visits);
-  raw_score_ = GetFinalRelevancyScore(topicality_score, frequency_score);
+  raw_score_ = GetFinalRelevancyScore(topicality_score, frequency_score,
+                                      *hqp_relevance_buckets_);
   raw_score_ =
       (raw_score_ <= kint32max) ? static_cast<int>(raw_score_) : kint32max;
 
   if (also_do_hup_like_scoring_ && can_inline_) {
     // HistoryURL-provider-like scoring gives any match that is
     // capable of being inlined a certain minimum score.  Some of these
     // are given a higher score that lets them be shown in inline.
     // This test here derives from the test in
     // HistoryURLProvider::PromoteMatchForInlineAutocomplete().
     const bool promote_to_inline = (row.typed_count() > 1) ||
@@ skipping 263 matching lines @@
     // due to this test would look stupid if shown to the user.
     if (term_scores[i] == 0)
       return 0;
     topicality_score += raw_term_score_to_topicality_score_[
         (term_scores[i] >= kMaxRawTermScore) ? (kMaxRawTermScore - 1) :
         term_scores[i]];
   }
   // TODO(mpearson): If there are multiple terms, consider taking the
   // geometric mean of per-term scores rather than the arithmetic mean.
 
-  return topicality_score / num_terms;
+  float final_topicality_score = topicality_score / num_terms;
+
+  // Demote all the URLs if the topicality score is less than threshold.
+  if (hqp_experimental_scoring_enabled_ &&
+      (final_topicality_score < topicality_threshold_)) {
+    return 0.0;
+  }
+
+  return final_topicality_score;
 }
 
 // static
 void ScoredHistoryMatch::FillInTermScoreToTopicalityScoreArray() {
   for (int term_score = 0; term_score < kMaxRawTermScore; ++term_score) {
     float topicality_score;
     if (term_score < 10) {
       // If the term scores less than 10 points (no full-credit hit, or
       // no combination of hits that score that well), then the topicality
       // score is linear in the term score.
@@ skipping 83 matching lines @@
     if (bookmarked)
       value_of_transition = std::max(value_of_transition, bookmark_value_);
     const float bucket_weight =
         GetRecencyScore((now - visits[i].first).InDays());
     summed_visit_points += (value_of_transition * bucket_weight);
   }
   return visits.size() * summed_visit_points / kMaxVisitsToScore;
 }
 
 // static
-float ScoredHistoryMatch::GetFinalRelevancyScore(float topicality_score,
-                                                 float frequency_score) {
+float ScoredHistoryMatch::GetFinalRelevancyScore(
+    float topicality_score,
+    float frequency_score,
+    const std::vector<ScoreMaxRelevance>& hqp_relevance_buckets) {
   if (topicality_score == 0)
     return 0;
   // Here's how to interpret intermediate_score: Suppose the omnibox
   // has one input term.  Suppose we have a URL for which the omnibox
   // input term has a single URL hostname hit at a word boundary.  (This
   // implies topicality_score = 1.0.).  Then the intermediate_score for
   // this URL will depend entirely on the frequency_score with
   // this interpretation:
   // - a single typed visit more than three months ago, no other visits -> 0.2
   // - a visit every three days, no typed visits -> 0.706
   // - a visit every day, no typed visits -> 0.916
   // - a single typed visit yesterday, no other visits -> 2.0
   // - a typed visit once a week -> 11.77
   // - a typed visit every three days -> 14.12
   // - at least ten typed visits today -> 20.0 (maximum score)
+  //
+  // The below code maps intermediate_score to the range [0, 1399].
+  // For example:
+  // HQP default scoring buckets: "1.5:600,12.0:1300,20.0:1399"

[Mark P, 2015/02/14 01:27:13]

I think you'll want your format to require having a 0.0 point set.  Don't
hard-code it to 400 as you do below.  This will allow you to try more types of
experiments later.
As you make this fix, you'll want to change this example string and the example
strings and default value string below.

[Ashok vardhan, 2015/02/17 01:23:52]

Make sense. Thought all the minimum scores for providers are not controlled from
params.

+  // We will linearly interpolate the scores between:
+  //      0 to 1.5    --> 400 to 600
+  //    1.5 to 12.0   --> 600 to 1300
+  //    12.0 to 20.0  --> 1300 to 1399
+  //       >= 20.0    --> 1399
+  //
+  // The score maxes out at 1400 (i.e., cannot beat a good inline result).

[Mark P, 2015/02/14 01:27:14]

1400 -> 1399
also, good inline result -> good inlineable result from HistoryURL provider

[Ashok vardhan, 2015/02/17 01:23:52]

Done.

+  //
+  // If experimental scoring is enabled, then the score buckets will be like:

[Mark P, 2015/02/14 01:27:14]

I don't know what line 592-593 are adding.  Is it merely that you wanted to give
another example of a scoring bucket string?  I'm not sure that's necessary.

[Ashok vardhan, 2015/02/17 01:23:52]

Done.

+  // HQP experimental scoring buckets: "1.5:600,5.0:900,12.0:1100,20.0:1300"
   const float intermediate_score = topicality_score * frequency_score;
-  // The below code maps intermediate_score to the range [0, 1399].
-  // The score maxes out at 1400 (i.e., cannot beat a good inline result).
-  if (intermediate_score <= 1) {
-    // Linearly extrapolate between 0 and 1.5 so 0 has a score of 400
-    // and 1.5 has a score of 600.
-    const float slope = (600 - 400) / (1.5f - 0.0f);
-    return 400 + slope * intermediate_score;
+
+  double base_intermediate_score = 0.0;

[Mark P, 2015/02/14 01:27:14]

Please comment the variables in this block more or give them better names (or
both!).  I can't figure out what you're doing without reading the code in
detail.

P.S.  I think having the 0 point specified in the config string will make the
code easier; you probably need two fewer variables.

[Ashok vardhan, 2015/02/17 01:23:52]

Acknowledged.

+  int base_hqp_score = 400;
+  double max_intermediate_score = base_intermediate_score;
+  int max_hqp_score = base_hqp_score;
+
+  // Find the threshold where intermediate score is greater than bucket.
+  for (size_t i = 0; i < hqp_relevance_buckets.size(); ++i) {
+    ScoreMaxRelevance hqp_bucket = hqp_relevance_buckets[i];

[Mark P, 2015/02/14 01:27:14]

const &

[Ashok vardhan, 2015/02/17 01:23:52]

Done.

+    max_intermediate_score = hqp_bucket.first;
+    max_hqp_score = hqp_bucket.second;
+    if (intermediate_score <= max_intermediate_score) {
+      const float slope = (
+          (max_hqp_score - base_hqp_score) /
+          (max_intermediate_score - base_intermediate_score));
+      const int final_hqp_score = (base_hqp_score +

[Mark P, 2015/02/14 01:27:13]

does the whole right side of the equals fit on one line?  If so, do that rather
than spread it over three.

[Mark P, 2015/02/14 01:27:14]

This function returns a float.  Don't force it to an int unnecessarily.

ditto below with other return value
(though in that case it matters less)

[Ashok vardhan, 2015/02/17 01:23:52]

Nope. And also i guess its easy to read if it is like this. Please let me know
your thoughts.

[Ashok vardhan, 2015/02/17 01:23:52]

Done.

[Ashok vardhan, 2015/02/17 01:23:52]

Done.

+                                   (slope * (intermediate_score -
+                                             base_intermediate_score)));
+      return std::min(final_hqp_score, max_hqp_score);

[Mark P, 2015/02/14 01:27:13]

Why is this min necessary?

[Ashok vardhan, 2015/02/17 01:23:52]

Done.

+    }
+    base_intermediate_score = max_intermediate_score;
+    base_hqp_score = max_hqp_score;
   }
-  if (intermediate_score <= 12.0) {
-    // Linearly extrapolate up to 12 so 12 has a score of 1300.
-    const float slope = (1300 - 600) / (12.0f - 1.5f);
-    return 600 + slope * (intermediate_score - 1.5);
-  }
-  // Linearly extrapolate so a score of 20 (or more) has a score of 1399.
-  // (Scores above 20 are possible for URLs that have multiple term hits
-  // in the URL and/or title and that are visited practically all
-  // the time using typed visits.  We don't attempt to distinguish
-  // between these very good results.)
-  const float slope = (1399 - 1300) / (20.0f - 12.0f);
-  return std::min(1399.0, 1300 + slope * (intermediate_score - 12.0));
+  // It will reach this stage when the score is > highest bucket score or
+  // when buckets are not specified. Return max_hqp_score.

[Mark P, 2015/02/14 01:27:14]

Buckets should never be not specified.  Correct this comment.

Perhaps even put a DCHECK(buckets.size > 0) at the beginning of the function, if
you feel like it.

[Ashok vardhan, 2015/02/17 01:23:52]

Done.

+  return max_hqp_score;
 }
 
+// static
+void ScoredHistoryMatch::InitializeHQPExperimentalParams() {
+  // These are default HQP relevance scoring buckets.
+  // See GetFinalRelevancyScore() for details.
+  std::string hqp_relevance_buckets_str = "1.5:600,12.0:1300,20.0:1399";
+
+  // Fetch the experiment params if they are any.
+  hqp_experimental_scoring_enabled_ =
+    OmniboxFieldTrial::HQPExperimentalScoringEnabled();
+
+  if (hqp_experimental_scoring_enabled_) {
+    // Add the topicality threshold from experiment params.
+    float hqp_experimental_topicality_threhold =
+      OmniboxFieldTrial::HQPExperimentalTopicalityThreshold();
+    if (hqp_experimental_topicality_threhold > 0)

[Mark P, 2015/02/14 01:27:14]

Why have a >0 test?  If you're checking for unspecified, use the -1 that you say
you return for unspecified parameters.

That said, I'm not convinced you need to check the return value here.  -1 is a
perfectly fine default to set the threshold_ variable to.

[Ashok vardhan, 2015/02/17 01:23:52]

Done.

+      topicality_threshold_ = hqp_experimental_topicality_threhold;
+
+    // Add the HQP experimental scoring buckets.
+    std::string hqp_experimental_scoring_buckets =
+      OmniboxFieldTrial::HQPExperimentalScoringBuckets();
+    if (!hqp_experimental_scoring_buckets.empty())
+      hqp_relevance_buckets_str = hqp_experimental_scoring_buckets;
+  }
+
+  // Parse the hqp_relevance_buckets_str string once and store them in vector
+  // which is easy to access.
+  hqp_relevance_buckets_ =
+      new std::vector<ScoredHistoryMatch::ScoreMaxRelevance>();
+  base::StringPairs kv_pairs;
+  if (base::SplitStringIntoKeyValuePairs(hqp_relevance_buckets_str,
+                                         ':', ',', &kv_pairs)) {
+    for (base::StringPairs::const_iterator it = kv_pairs.begin();
+         it != kv_pairs.end(); ++it) {
+      ScoreMaxRelevance bucket;
+      base::StringToDouble(it->first, &bucket.first);

[Mark P, 2015/02/14 01:27:14]

minor nit: consider DCHECKING the return value for these

[Ashok vardhan, 2015/02/17 01:23:52]

DCHECK seems to be not working here.

  std::string x = "1";
  int a = 0;
  int b = 0; 
  base::StringToInt(x, &a);
  DCHECK(base::StringToInt(x, &b));                                            
  LOG(INFO) << "a is : " << a << std::endl;
  LOG(INFO) << "b is : " << b << std::endl;

Logs:
a is : 1
b is : 0

Am i missing anything here ? I used CHECK in our code-base, but this is strange.
I can use if condition checkings but not sure why this is not working.

+      base::StringToInt(it->second, &bucket.second);
+      hqp_relevance_buckets_->push_back(bucket);
+    }
+  }
+}
+
+// static
 void ScoredHistoryMatch::Init() {
   if (initialized_)
     return;
   also_do_hup_like_scoring_ = false;
   // When doing HUP-like scoring, don't allow a non-inlineable match
   // to beat the score of good inlineable matches.  This is a problem
   // because if a non-inlineable match ends up with the highest score
   // from HistoryQuick provider, all HistoryQuick matches get demoted
   // to non-inlineable scores (scores less than 1200).  Without
   // HUP-like-scoring, these results would actually come from the HUP
   // and not be demoted, thus outscoring the demoted HQP results.
   // When the HQP provides these, we need to clamp the non-inlineable
   // results to preserve this behavior.
   if (also_do_hup_like_scoring_) {
     max_assigned_score_for_non_inlineable_matches_ =
         HistoryURLProvider::kScoreForBestInlineableResult - 1;
   }
   bookmark_value_ = OmniboxFieldTrial::HQPBookmarkValue();
   allow_tld_matches_ = OmniboxFieldTrial::HQPAllowMatchInTLDValue();
   allow_scheme_matches_ = OmniboxFieldTrial::HQPAllowMatchInSchemeValue();
+
+  // Initialize the HQP Experimental scoring params.
+  InitializeHQPExperimentalParams();
+
   initialized_ = true;
 }
 
 }  // namespace history