Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2585263002:
[NTP::SectionOrder] First version of click based category ranker. (Closed)
Description[NTP::SectionOrder] First version of click based category ranker.
This CL adds an implementation of CategoryRanker based on clicks.
BUG=674851
Committed: https://crrev.com/9f8aa5f8fc0acdbb5f473687f547edcc342df687
Cr-Commit-Position: refs/heads/master@{#440369}
Patch Set 1 #
Total comments: 51
Patch Set 2 : rebase. #Patch Set 3 : treib@ & tschumann@ comments + tests. #
Total comments: 18
Patch Set 4 : rebase. #Patch Set 5 : tschumann@ comments. #
Total comments: 16
Patch Set 6 : jkrcal@ & tschumann@ comments + 1 test. #Patch Set 7 : rebase. #
Total comments: 14
Patch Set 8 : rebase. #Patch Set 9 : tschumann@ & bauerb@ comments. #Dependent Patchsets: Messages
Total messages: 50 (31 generated)
vitaliii@chromium.org changed reviewers: + treib@chromium.org
Hi treib@, This CL is still very raw, but it seems to work already. Could you please have a rather high level look if you have time, so that you are aware in which direction I am moving (and could suggest something)? https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... File components/ntp_snippets/category_rankers/click_based_category_ranker.cc (right): https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:199: return std::find_if(ordered_categories_.cbegin(), ordered_categories_.cend(), Is there any way to reuse FindCategory here, while keeping FindCategory non const? https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... File components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc (right): https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. Ignore this file yet.
https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... File components/ntp_snippets/category_rankers/click_based_category_ranker.cc (right): https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:21: // TODO(vitaliii): Implement clicks decay. unless you plan to implement them in this CL, the bug tracker would be a better place to document the missing pieces. If the CL works as is, I'd be in favor of keeping it small and add those in a separate CL. (Also other folks can jump in and help implementing those ;-)). https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:49: if (!ContainsCategory(left)) { this should be a DCHECK(ContainsCategory(...)); so that it gets optimized out in opt builds. https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:87: if (!ContainsCategory(category)) { DCHECK() https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:96: if (current != ordered_categories_.begin()) { let's move this in a helper function like UpdateRanking(current). https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:99: if (current->clicks - previous->clicks >= threshold) { nit: for me current->clicks >= previous_clicks + threshold) is more intuitive. Might just be me though... https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:119: int index = category_position - ordered_categories_.cbegin(); nit: i'd use the term 'rank' to denote a position in the category ordering. https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:122: (kIncreasedClickThresholdCategoryCount - index); we could also move this into a constant: const int topEntryThresholdFactor = std::max(kIncreasedClickThresholdCategoryCount - index, 0); return kClickThreshold + kClickPerPositionThresholdIncrease * topEntryThresholdFactor; On a naming note, maybe we can find some more catchy term for the increased threshold concept. Something like "inertia", e.g. topEntryInertia? https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:165: DCHECK(success) << "Failed to parse dismissed id from prefs param " nit: if we move the parsing in to a helper function, these lines could turn into: if (!value->GetAs...)) { DCHECK(...); return false; } At the calling side, we could simply do: std::vector<RankedCategory> loaded_data; if (!ParsePrefValue(pref_service_->GetList(prefs::kClickBasedCategoryRankerOrderWithClicks), &loaded_data) { return false; } ordered_categories_ = loaded_data; return true; Or alternatively, this function could be that helper, i.e. return a bool and take a vector<>* :-) https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:199: return std::find_if(ordered_categories_.cbegin(), ordered_categories_.cend(), On 2016/12/19 13:37:51, vitaliii wrote: > Is there any way to reuse FindCategory here, while keeping FindCategory non > const? the only way I see is to make FindCategory() const by returning a const-pointer -- but you want to swap pointers later... Nope. But it's also not too bad to duplicate that. I'm just wondering if a plain old loop would be easier to read: for (const auto& ranked_category : ordered_categories_) { if (category == ranked_category.category) { return true; } } return false; https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... File components/ntp_snippets/category_rankers/click_based_category_ranker.h (right): https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.h:21: // TODO(vitaliii): Briefly tell main ideas. ideally we'd link the design doc here (but I guess we can't do that in Chromium). Actually, I think this is sufficient for a header comment. Let's rather explain the trickier parts inside the .cc file next to where it gets implemented. Spoke too soon :-) One thing I would mention is the life-cycle. That categories get registered and statistics about their usage persisted in prefs and reloaded the next time. https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.h:24: ClickBasedCategoryRanker(PrefService* pref_service); explicit https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.h:33: static void RegisterProfilePrefs(PrefRegistrySimple* registry); please document. When should it be called? https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.h:47: please drop the blank lines between functions. They just take up space. IMO, there's no need to document private functions (== implementation details) in the header file (which is mainly concerned about the public interface).
https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... File components/ntp_snippets/category_rankers/click_based_category_ranker.cc (right): https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:28: const int kClickThreshold = 5; I'm not sure I understand the comment - is this the number of *extra* clicks a category needs before we'll change the order? https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:32: const int kIncreasedClickThresholdCategoryCount = 3; No idea what these two will do :) https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:49: if (!ContainsCategory(left)) { On 2016/12/19 14:10:44, tschumann wrote: > this should be a > DCHECK(ContainsCategory(...)); > so that it gets optimized out in opt builds. The intention here is a bit different: LOG(DFATAL) says "we're not quite confident enough to make this an assertion (i.e. DCHECK), but we still want to be notified if it happens". That's not a great situation to be in, but IMO a DCHECK isn't justified here. If someone forgets to register their category with the ranker, then a crash in the ranker isn't very helpful. https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:53: if (!ContainsCategory(left)) { right https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:96: if (current != ordered_categories_.begin()) { On 2016/12/19 14:10:43, tschumann wrote: > let's move this in a helper function like UpdateRanking(current). Let's make it UpdateRanking(category) so we don't have to pass iterators around. https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:131: // Add all local categories in a fixed order. This is shared with the constant ranker. Should we put it in a common place somewhere? https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:171: success = dictionary->GetInteger(kClicksKey, &clicks) && success; The trailing "&& success" is super easy to miss... https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... File components/ntp_snippets/category_rankers/click_based_category_ranker.h (right): https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.h:19: // An implementation of a CategoryRanker based on a number of clicks. Initial "a number of clicks" is quite unspecific. "the number of clicks per category"? https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.h:21: // TODO(vitaliii): Briefly tell main ideas. On 2016/12/19 14:10:44, tschumann wrote: > ideally we'd link the design doc here (but I guess we can't do that in > Chromium). Actually, I think this is sufficient for a header comment. Let's > rather explain the trickier parts inside the .cc file next to where it gets > implemented. We *could* link to design docs, since they should ideally be public, but we generally don't. The reason is that they're all but guaranteed to become outdated very soon. When someone changes the code, there's at least a reasonable chance they'll also update the relevant comments, but that'll never happen for a design doc. So, if it's really necessary to explain the details, then that should happen in comments (in the .cc file). > Spoke too soon :-) > One thing I would mention is the life-cycle. That categories get registered and > statistics about their usage persisted in prefs and reloaded the next time. +1 here :) https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.h:31: void OnSuggestionOpened(Category category) override; One high-level comment: This ranker works on absolute number of clicks, not CTR. That means a category which shows up only occasionally, but then has 100% CTR, will be ranked rather low. So I think ranking based on CTR would be better. WDYT? https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.h:47: On 2016/12/19 14:10:44, tschumann wrote: > please drop the blank lines between functions. They just take up space. IMO, > there's no need to document private functions (== implementation details) in the > header file (which is mainly concerned about the public interface). Well, we do have lots of comments on private methods in other headers (which, arguably, is a smell). I sometimes like newlines to delimit groups of related methods - e.g. ReadOrder and WriteOrder should be a group, i.e. no newline between. Also, the bike shed should be sky blue.
vitaliii@chromium.org changed reviewers: + tschumann@chromium.org
answer to a high level comment. https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... File components/ntp_snippets/category_rankers/click_based_category_ranker.h (right): https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.h:31: void OnSuggestionOpened(Category category) override; On 2016/12/19 15:11:29, Marc Treib (OOO until Jan 12) wrote: > One high-level comment: This ranker works on absolute number of clicks, not CTR. > That means a category which shows up only occasionally, but then has 100% CTR, > will be ranked rather low. > So I think ranking based on CTR would be better. WDYT? This looks like a good idea, especially in the mentioned case (it may be important to notice this rarely visible section (e.g. Physical Web), which would be terribly inconvenient if there are 3-4 section before it), however, it also brings some difficulties: 1) Not clear how to incorporate the threshold. If you use e.g. 5% this may be 10 clicks or 1000 clicks. 2) Value of a click decreases with time; 3) It may be biased in favor of rarely visible sections. Imagine that there are a rarely visible section with CTR 5/10 and articles section with CTR 200/1000. Articles need 300 clicks to reach the other section. However, one could claim that because the section is rarely visible, then misranking it is not a big problem. Also imagine that usually 5 sections are visible on the NTP. Then click on each of this section will decrease articles CTR. This could be resolved by fairly counting impressions (when user scroll to the section). Overall, it is definitely worth considering, but I feel like we are more sound with absolute clicks in terms of getting it into M57. I would prefer finish it and then considering CTR based one. tschumann@, WDYT?
https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... File components/ntp_snippets/category_rankers/click_based_category_ranker.cc (right): https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:49: if (!ContainsCategory(left)) { On 2016/12/19 15:11:29, Marc Treib (OOO until Jan 12) wrote: > On 2016/12/19 14:10:44, tschumann wrote: > > this should be a > > DCHECK(ContainsCategory(...)); > > so that it gets optimized out in opt builds. > > The intention here is a bit different: LOG(DFATAL) says "we're not quite > confident enough to make this an assertion (i.e. DCHECK), but we still want to > be notified if it happens". > That's not a great situation to be in, but IMO a DCHECK isn't justified here. If > someone forgets to register their category with the ranker, then a crash in the > ranker isn't very helpful. Right -- sorry, missed that. I'm also not sure if dying is the best way here. What worries me a bit though, is that the more complicated cases are unlikely to be covered by unit-tests. I guess we don't do on-device tests (like testing the whole thing) with non-optimized binaries, hm? https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... File components/ntp_snippets/category_rankers/click_based_category_ranker.h (right): https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.h:31: void OnSuggestionOpened(Category category) override; On 2016/12/19 16:00:35, vitaliii wrote: > On 2016/12/19 15:11:29, Marc Treib (OOO until Jan 12) wrote: > > One high-level comment: This ranker works on absolute number of clicks, not > CTR. > > That means a category which shows up only occasionally, but then has 100% CTR, > > will be ranked rather low. > > So I think ranking based on CTR would be better. WDYT? > > This looks like a good idea, especially in the mentioned case (it may be > important to notice this rarely visible section (e.g. Physical Web), which would > be terribly inconvenient if there are 3-4 section before it), however, it also > brings some difficulties: > 1) Not clear how to incorporate the threshold. If you use e.g. 5% this may be 10 > clicks or 1000 clicks. > 2) Value of a click decreases with time; > 3) It may be biased in favor of rarely visible sections. Imagine that there are > a rarely visible section with CTR 5/10 and articles section with CTR 200/1000. > Articles need 300 clicks to reach the other section. However, one could claim > that because the section is rarely visible, then misranking it is not a big > problem. > Also imagine that usually 5 sections are visible on the NTP. Then click on each > of this section will decrease articles CTR. This could be resolved by fairly > counting impressions (when user scroll to the section). > > Overall, it is definitely worth considering, but I feel like we are more sound > with absolute clicks in terms of getting it into M57. I would prefer finish it > and then considering CTR based one. > > tschumann@, WDYT? I agree we should start simple first. We can experiment with an additional "impression" signal, i.e. we would maintain a similar counter which tracks the impressions and we can use them for scoring, too. However, we should now focus on getting the simplest solution which can possibly work in and then iterate. Even if we go the CTR way, it doesn't solve the problem of discoverability. If a user never sees a section, then how do we compare this? both counts and impression counters would be empty. We only have limited discoverable space and we have to live with that. With Chrome Home, some sections will move out, so maybe that helps. What I can better imagine is that we do some experiments with specific sections, e.g. for some fraction of users, we boost a category to position on the top (no idea, 1 or 2 or 3) and then just see how the CTR it gets at that position compares against other categories at that position. This should help us with the seeding bias and could be sort of a promotion for new suggestions which appear to perform well. Seems like we should move this discussion in a more prominent place --> what about the ranker's design doc?
The CQ bit was checked by vitaliii@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...
Hi, I addressed your comments, PTAL. https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... File components/ntp_snippets/category_rankers/click_based_category_ranker.cc (right): https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:21: // TODO(vitaliii): Implement clicks decay. On 2016/12/19 14:10:43, tschumann wrote: > unless you plan to implement them in this CL, the bug tracker would be a better > place to document the missing pieces. > If the CL works as is, I'd be in favor of keeping it small and add those in a > separate CL. (Also other folks can jump in and help implementing those ;-)). Done. https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:28: const int kClickThreshold = 5; On 2016/12/19 15:11:29, Marc Treib (OOO until Jan 12) wrote: > I'm not sure I understand the comment - is this the number of *extra* clicks a > category needs before we'll change the order? Done. The idea with "extra" is great. https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:32: const int kIncreasedClickThresholdCategoryCount = 3; On 2016/12/19 15:11:29, Marc Treib (OOO until Jan 12) wrote: > No idea what these two will do :) Done. I added comments. https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:49: if (!ContainsCategory(left)) { On 2016/12/19 17:12:34, tschumann wrote: > On 2016/12/19 15:11:29, Marc Treib (OOO until Jan 12) wrote: > > On 2016/12/19 14:10:44, tschumann wrote: > > > this should be a > > > DCHECK(ContainsCategory(...)); > > > so that it gets optimized out in opt builds. > > > > The intention here is a bit different: LOG(DFATAL) says "we're not quite > > confident enough to make this an assertion (i.e. DCHECK), but we still want to > > be notified if it happens". > > That's not a great situation to be in, but IMO a DCHECK isn't justified here. > If > > someone forgets to register their category with the ranker, then a crash in > the > > ranker isn't very helpful. > > Right -- sorry, missed that. I'm also not sure if dying is the best way here. > What worries me a bit though, is that the more complicated cases are unlikely to > be covered by unit-tests. > I guess we don't do on-device tests (like testing the whole thing) with > non-optimized binaries, hm? Ack. I do not do on-device tests except manually confirming that CL works as expected. https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:53: if (!ContainsCategory(left)) { On 2016/12/19 15:11:29, Marc Treib (OOO until Jan 12) wrote: > right Done. https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:87: if (!ContainsCategory(category)) { On 2016/12/19 14:10:43, tschumann wrote: > DCHECK() Ack. Same argument as in Compare. Someone may forget registering the category. https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:96: if (current != ordered_categories_.begin()) { On 2016/12/19 15:11:29, Marc Treib (OOO until Jan 12) wrote: > On 2016/12/19 14:10:43, tschumann wrote: > > let's move this in a helper function like UpdateRanking(current). > > Let's make it UpdateRanking(category) so we don't have to pass iterators around. Done. https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:96: if (current != ordered_categories_.begin()) { On 2016/12/19 14:10:43, tschumann wrote: > let's move this in a helper function like UpdateRanking(current). Done. https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:99: if (current->clicks - previous->clicks >= threshold) { On 2016/12/19 14:10:44, tschumann wrote: > nit: for me current->clicks >= previous_clicks + threshold) is more intuitive. > Might just be me though... Done. https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:119: int index = category_position - ordered_categories_.cbegin(); On 2016/12/19 14:10:44, tschumann wrote: > nit: i'd use the term 'rank' to denote a position in the category ordering. Ack. "Index" hints that the number is 0 based, while with "rank" it is not clear. https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:122: (kIncreasedClickThresholdCategoryCount - index); On 2016/12/19 14:10:43, tschumann wrote: > we could also move this into a constant: > const int topEntryThresholdFactor = > std::max(kIncreasedClickThresholdCategoryCount - index, 0); > return kClickThreshold + kClickPerPositionThresholdIncrease * > topEntryThresholdFactor; > > On a naming note, maybe we can find some more catchy term for the increased > threshold concept. Something like "inertia", e.g. topEntryInertia? Acknowledged. I prefer "if", because "max" with difference is harder to parse and understand. As for the naming, I like the idea of more catchy term, but I have no ideas and "inertia" feels a bit weird. If I come up with anything, I will change the name. https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:131: // Add all local categories in a fixed order. On 2016/12/19 15:11:29, Marc Treib (OOO until Jan 12) wrote: > This is shared with the constant ranker. Should we put it in a common place > somewhere? Done. https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:165: DCHECK(success) << "Failed to parse dismissed id from prefs param " On 2016/12/19 14:10:43, tschumann wrote: > nit: if we move the parsing in to a helper function, these lines could turn > into: > if (!value->GetAs...)) { > DCHECK(...); > return false; > } > At the calling side, we could simply do: > > std::vector<RankedCategory> loaded_data; > if > (!ParsePrefValue(pref_service_->GetList(prefs::kClickBasedCategoryRankerOrderWithClicks), > &loaded_data) { > return false; > } > ordered_categories_ = loaded_data; > return true; > > Or alternatively, this function could be that helper, i.e. return a bool and > take a vector<>* :-) Done. I did the latter. https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:171: success = dictionary->GetInteger(kClicksKey, &clicks) && success; On 2016/12/19 15:11:29, Marc Treib (OOO until Jan 12) wrote: > The trailing "&& success" is super easy to miss... Done. https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:199: return std::find_if(ordered_categories_.cbegin(), ordered_categories_.cend(), On 2016/12/19 14:10:43, tschumann wrote: > On 2016/12/19 13:37:51, vitaliii wrote: > > Is there any way to reuse FindCategory here, while keeping FindCategory non > > const? > > the only way I see is to make FindCategory() const by returning a const-pointer > -- but you want to swap pointers later... > Nope. But it's also not too bad to duplicate that. I'm just wondering if a plain > old loop would be easier to read: > > for (const auto& ranked_category : ordered_categories_) { > if (category == ranked_category.category) { > return true; > } > } > return false; Done. Rewrote ContainsCategory with a |for| loop. https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... File components/ntp_snippets/category_rankers/click_based_category_ranker.h (right): https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.h:19: // An implementation of a CategoryRanker based on a number of clicks. Initial On 2016/12/19 15:11:29, Marc Treib (OOO until Jan 12) wrote: > "a number of clicks" is quite unspecific. "the number of clicks per category"? Done. https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.h:21: // TODO(vitaliii): Briefly tell main ideas. On 2016/12/19 14:10:44, tschumann wrote: > ideally we'd link the design doc here (but I guess we can't do that in > Chromium). Actually, I think this is sufficient for a header comment. Let's > rather explain the trickier parts inside the .cc file next to where it gets > implemented. > > Spoke too soon :-) > One thing I would mention is the life-cycle. That categories get registered and > statistics about their usage persisted in prefs and reloaded the next time. Done. https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.h:24: ClickBasedCategoryRanker(PrefService* pref_service); On 2016/12/19 14:10:44, tschumann wrote: > explicit Done. https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.h:31: void OnSuggestionOpened(Category category) override; On 2016/12/19 17:12:34, tschumann wrote: > On 2016/12/19 16:00:35, vitaliii wrote: > > On 2016/12/19 15:11:29, Marc Treib (OOO until Jan 12) wrote: > > > One high-level comment: This ranker works on absolute number of clicks, not > > CTR. > > > That means a category which shows up only occasionally, but then has 100% > CTR, > > > will be ranked rather low. > > > So I think ranking based on CTR would be better. WDYT? > > > > This looks like a good idea, especially in the mentioned case (it may be > > important to notice this rarely visible section (e.g. Physical Web), which > would > > be terribly inconvenient if there are 3-4 section before it), however, it also > > brings some difficulties: > > 1) Not clear how to incorporate the threshold. If you use e.g. 5% this may be > 10 > > clicks or 1000 clicks. > > 2) Value of a click decreases with time; > > 3) It may be biased in favor of rarely visible sections. Imagine that there > are > > a rarely visible section with CTR 5/10 and articles section with CTR 200/1000. > > Articles need 300 clicks to reach the other section. However, one could claim > > that because the section is rarely visible, then misranking it is not a big > > problem. > > Also imagine that usually 5 sections are visible on the NTP. Then click on > each > > of this section will decrease articles CTR. This could be resolved by fairly > > counting impressions (when user scroll to the section). > > > > Overall, it is definitely worth considering, but I feel like we are more sound > > with absolute clicks in terms of getting it into M57. I would prefer finish it > > and then considering CTR based one. > > > > tschumann@, WDYT? > > I agree we should start simple first. We can experiment with an additional > "impression" signal, i.e. we would maintain a similar counter which tracks the > impressions and we can use them for scoring, too. > However, we should now focus on getting the simplest solution which can possibly > work in and then iterate. > > Even if we go the CTR way, it doesn't solve the problem of discoverability. If a > user never sees a section, then how do we compare this? both counts and > impression counters would be empty. We only have limited discoverable space and > we have to live with that. With Chrome Home, some sections will move out, so > maybe that helps. > What I can better imagine is that we do some experiments with specific sections, > e.g. for some fraction of users, we boost a category to position on the top (no > idea, 1 or 2 or 3) and then just see how the CTR it gets at that position > compares against other categories at that position. This should help us with the > seeding bias and could be sort of a promotion for new suggestions which appear > to perform well. Seems like we should move this discussion in a more prominent > place --> what about the ranker's design doc? Ack. Let's finish this one and then consider CTR based one. https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.h:33: static void RegisterProfilePrefs(PrefRegistrySimple* registry); On 2016/12/19 14:10:44, tschumann wrote: > please document. When should it be called? Acknowledged. Every class (I saw) using prefs has this function and it is called only from browser_prefs.cc, so I am not sure whether the comment is really needed. https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.h:47: On 2016/12/19 14:10:44, tschumann wrote: > please drop the blank lines between functions. They just take up space. IMO, > there's no need to document private functions (== implementation details) in the > header file (which is mainly concerned about the public interface). Done. https://codereview.chromium.org/2585263002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.h:47: On 2016/12/19 14:10:44, tschumann wrote: > please drop the blank lines between functions. They just take up space. IMO, > there's no need to document private functions (== implementation details) in the > header file (which is mainly concerned about the public interface). Done.
vitaliii@chromium.org changed reviewers: + jkrcal@chromium.org
Hi jkrcal@, Since treib@ is OOO, could you have a look?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by vitaliii@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...
overall looks good, mostly about naming and keeping test-code out of the interface. https://codereview.chromium.org/2585263002/diff/40001/components/ntp_snippets... File components/ntp_snippets/category_rankers/click_based_category_ranker.cc (right): https://codereview.chromium.org/2585263002/diff/40001/components/ntp_snippets... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:32: const int kExtraClickPerPositionThresholdIncrease = 2; the constant name is misleading -- it's a multiplier / factor. I think we should introduce some better concept here. "ExtraClick" doesn't really have a lot of meaning. At some point we should also start thinking about a "score" concept. Because after a while it's actually not comparable to clicks anymore (time-decay and stuff). the extra-click's purpose is to stabilize the ranking. Just brain-storming here, maybe you like some of them: PassingTax PassingPenalty kIncreasedExtraClickThresholdCategoryCount would then become MaxPassingTaxRank and kExtraClickPerPositionThresholdIncrease could be PassingTaxFactor (not sure we need to mention the rank here -- i wouldn't use the term position; rank has pretty precise semantics when it comes to ranking :-). https://codereview.chromium.org/2585263002/diff/40001/components/ntp_snippets... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:195: << " into dictionary."; shouldn't we return false in case of !success? https://codereview.chromium.org/2585263002/diff/40001/components/ntp_snippets... File components/ntp_snippets/category_rankers/click_based_category_ranker.h (right): https://codereview.chromium.org/2585263002/diff/40001/components/ntp_snippets... components/ntp_snippets/category_rankers/click_based_category_ranker.h:39: void ClearCategoriesForTesting(); why would we need this? Can't we just re-create an instance in a test? https://codereview.chromium.org/2585263002/diff/40001/components/ntp_snippets... components/ntp_snippets/category_rankers/click_based_category_ranker.h:41: static int GetIncreasedExtraClickThresholdCategoryCountForTesting(); this smells. I'd rather expose the constants in the header. If you want to hide this constants behind functions, then move them off the class. if you want to signal it's internal, consider putting them into an internal namespace. https://codereview.chromium.org/2585263002/diff/40001/components/ntp_snippets... File components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc (right): https://codereview.chromium.org/2585263002/diff/40001/components/ntp_snippets... components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc:56: GetIncreasedExtraClickThresholdCategoryCountForTesting(); let's inline this -- less indirection :-) Same for GetExtracClickThreshold(). https://codereview.chromium.org/2585263002/diff/40001/components/ntp_snippets... components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc:182: ranker()->ClearCategoriesForTesting(); can we find a way to test this through the public API?
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 vitaliii@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...
I addressed tschumann@ comments, PTAL. https://codereview.chromium.org/2585263002/diff/40001/components/ntp_snippets... File components/ntp_snippets/category_rankers/click_based_category_ranker.cc (right): https://codereview.chromium.org/2585263002/diff/40001/components/ntp_snippets... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:32: const int kExtraClickPerPositionThresholdIncrease = 2; On 2016/12/20 16:27:02, tschumann wrote: > the constant name is misleading -- it's a multiplier / factor. > I think we should introduce some better concept here. "ExtraClick" doesn't > really have a lot of meaning. At some point we should also start thinking about > a "score" concept. Because after a while it's actually not comparable to clicks > anymore (time-decay and stuff). > > the extra-click's purpose is to stabilize the ranking. > Just brain-storming here, maybe you like some of them: > PassingTax > PassingPenalty > > kIncreasedExtraClickThresholdCategoryCount would then become > MaxPassingTaxRank > > and kExtraClickPerPositionThresholdIncrease could be PassingTaxFactor (not sure > we need to mention the rank here -- i wouldn't use the term position; rank has > pretty precise semantics when it comes to ranking :-). "Tax" implies decrease. I rewrote using "boundary width" and "top categories". I like boundary width, because then a category is moved upwards only when it crosses boundary of the previous category; before it is either behind or on the boundary. https://codereview.chromium.org/2585263002/diff/40001/components/ntp_snippets... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:195: << " into dictionary."; On 2016/12/20 16:27:03, tschumann wrote: > shouldn't we return false in case of !success? DCHECKs should not be handled. Why do you think we will hit these DCHECKs? The only case I see is when we change the format of prefs data, but then this must be handled accordingly and DCHECKs will be removed. BTW, should I store some data version marker, so that we could parse it easily if we change the format? https://codereview.chromium.org/2585263002/diff/40001/components/ntp_snippets... File components/ntp_snippets/category_rankers/click_based_category_ranker.h (right): https://codereview.chromium.org/2585263002/diff/40001/components/ntp_snippets... components/ntp_snippets/category_rankers/click_based_category_ranker.h:39: void ClearCategoriesForTesting(); On 2016/12/20 16:27:03, tschumann wrote: > why would we need this? Can't we just re-create an instance in a test? Done. I removed the function. It was needed to ensure that we are testing the class with "top" categories, however, now I use default order for this. https://codereview.chromium.org/2585263002/diff/40001/components/ntp_snippets... components/ntp_snippets/category_rankers/click_based_category_ranker.h:41: static int GetIncreasedExtraClickThresholdCategoryCountForTesting(); On 2016/12/20 16:27:03, tschumann wrote: > this smells. I'd rather expose the constants in the header. > If you want to hide this constants behind functions, then move them off the > class. > if you want to signal it's internal, consider putting them into an internal > namespace. Done. As per our offline discussion, I made methods public without "ForTesting", because they will be controlled via Finch and we won't be able to expose them as constants then. https://codereview.chromium.org/2585263002/diff/40001/components/ntp_snippets... File components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc (right): https://codereview.chromium.org/2585263002/diff/40001/components/ntp_snippets... components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc:56: GetIncreasedExtraClickThresholdCategoryCountForTesting(); On 2016/12/20 16:27:03, tschumann wrote: > let's inline this -- less indirection :-) > Same for GetExtracClickThreshold(). Done. https://codereview.chromium.org/2585263002/diff/40001/components/ntp_snippets... components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc:182: ranker()->ClearCategoriesForTesting(); On 2016/12/20 16:27:03, tschumann wrote: > can we find a way to test this through the public API? Done. Yes, I use default order now.
lgtm with nits (and one non-nit that can be left for another CL). https://codereview.chromium.org/2585263002/diff/40001/components/ntp_snippets... File components/ntp_snippets/category_rankers/click_based_category_ranker.cc (right): https://codereview.chromium.org/2585263002/diff/40001/components/ntp_snippets... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:32: const int kExtraClickPerPositionThresholdIncrease = 2; On 2016/12/21 09:25:01, vitaliii wrote: > On 2016/12/20 16:27:02, tschumann wrote: > > the constant name is misleading -- it's a multiplier / factor. > > I think we should introduce some better concept here. "ExtraClick" doesn't > > really have a lot of meaning. At some point we should also start thinking > about > > a "score" concept. Because after a while it's actually not comparable to > clicks > > anymore (time-decay and stuff). > > > > the extra-click's purpose is to stabilize the ranking. > > Just brain-storming here, maybe you like some of them: > > PassingTax > > PassingPenalty > > > > kIncreasedExtraClickThresholdCategoryCount would then become > > MaxPassingTaxRank > > > > and kExtraClickPerPositionThresholdIncrease could be PassingTaxFactor (not > sure > > we need to mention the rank here -- i wouldn't use the term position; rank has > > pretty precise semantics when it comes to ranking :-). > > "Tax" implies decrease. > > I rewrote using "boundary width" and "top categories". > I like boundary width, because then a category is moved upwards only when it > crosses boundary of the previous category; before it is either behind or on the > boundary. I am not happy about "boundary width", I like "passing". What about "passing margin"? I do not feel strongly. If Tim is fine with "boundary width", I am also fine. https://codereview.chromium.org/2585263002/diff/80001/components/ntp_snippets... File components/ntp_snippets/category_rankers/click_based_category_ranker.cc (right): https://codereview.chromium.org/2585263002/diff/80001/components/ntp_snippets... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:36: // which is then combined with a top category position. Did not understand at all. My suggestion: // The increase of boundary width for each top category compared to the next category (e.g. the first top category has boundary width larger than the second top category, the last top category has it larger than the first non-top category). const int kTopCategoryBoundaryWidthIcreasePerPosition = 2; // clicks This may be actually close to your original proposal, maybe it's better to agree with Tim before any further renamings :) https://codereview.chromium.org/2585263002/diff/80001/components/ntp_snippets... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:47: RestoreDefaultOrder(); This does not address the situation if there is a correct order in prefs but we introduce a new category. You will load the previous order correctly and the ranker will never count clicks for the new category. If you want to leave it for a follow-up CL, just file a bug and add a TODO here. https://codereview.chromium.org/2585263002/diff/80001/components/ntp_snippets... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:161: void ClickBasedCategoryRanker::UpdateRanking(Category category) { nit: I do not think this is a good abstraction / naming. This function is correct only assuming that: - the clicks for |category| have increased; - the clicks for any other category have not changed. Thus, it is directly bound to its only use case at the moment: OnSuggestionOpened(). I suggest to rename it to AddClick(Category category) end incorporate lines 100 - 105 in this function. https://codereview.chromium.org/2585263002/diff/80001/components/ntp_snippets... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:171: // It is indended to move only by one position at a time in order to avoid nit: intended https://codereview.chromium.org/2585263002/diff/80001/components/ntp_snippets... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:191: << "Failed to parse dismissed id from prefs param " update the error message https://codereview.chromium.org/2585263002/diff/80001/components/ntp_snippets... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:228: bool ClickBasedCategoryRanker::ContainsCategory(Category category) const { nit: why not implementing using: return (FindCategory(category) != ordered_categories_.end()); https://codereview.chromium.org/2585263002/diff/80001/components/ntp_snippets... File components/ntp_snippets/category_rankers/click_based_category_ranker.h (right): https://codereview.chromium.org/2585263002/diff/80001/components/ntp_snippets... components/ntp_snippets/category_rankers/click_based_category_ranker.h:40: // category upwards. See .cc file for more details. I would omit the second sentence (also below), this is kind of implied for every function.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2585263002/diff/40001/components/ntp_snippets... File components/ntp_snippets/category_rankers/click_based_category_ranker.cc (right): https://codereview.chromium.org/2585263002/diff/40001/components/ntp_snippets... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:32: const int kExtraClickPerPositionThresholdIncrease = 2; On 2016/12/21 10:38:26, jkrcal wrote: > On 2016/12/21 09:25:01, vitaliii wrote: > > On 2016/12/20 16:27:02, tschumann wrote: > > > the constant name is misleading -- it's a multiplier / factor. > > > I think we should introduce some better concept here. "ExtraClick" doesn't > > > really have a lot of meaning. At some point we should also start thinking > > about > > > a "score" concept. Because after a while it's actually not comparable to > > clicks > > > anymore (time-decay and stuff). > > > > > > the extra-click's purpose is to stabilize the ranking. > > > Just brain-storming here, maybe you like some of them: > > > PassingTax > > > PassingPenalty > > > > > > kIncreasedExtraClickThresholdCategoryCount would then become > > > MaxPassingTaxRank > > > > > > and kExtraClickPerPositionThresholdIncrease could be PassingTaxFactor (not > > sure > > > we need to mention the rank here -- i wouldn't use the term position; rank > has > > > pretty precise semantics when it comes to ranking :-). > > > > "Tax" implies decrease. > > > > I rewrote using "boundary width" and "top categories". > > I like boundary width, because then a category is moved upwards only when it > > crosses boundary of the previous category; before it is either behind or on > the > > boundary. > > I am not happy about "boundary width", I like "passing". > What about "passing margin"? > > I do not feel strongly. If Tim is fine with "boundary width", I am also fine. boundary isn't too great but already better. "tax means decrease": Well, tax means a certain way of punishment. So if you want to pass, you also need to pay that tax. The metaphor doesn't work 100% as you don't actually pay it; the score won't get reduced. But the overall image gives the whole thing a sense. PassingMargin also sounds good. Maybe PassingGap ? https://codereview.chromium.org/2585263002/diff/40001/components/ntp_snippets... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:195: << " into dictionary."; On 2016/12/21 09:25:01, vitaliii wrote: > On 2016/12/20 16:27:03, tschumann wrote: > > shouldn't we return false in case of !success? > > DCHECKs should not be handled. > Why do you think we will hit these DCHECKs? > The only case I see is when we change the format of prefs data, but then this > must be handled accordingly and DCHECKs will be removed. > > BTW, should I store some data version marker, so that we could parse it easily > if we change the format? no, don't worry about versioning. There are other ways to deal with that (we could also use a new key). One scenario is data corruption (we're storing it on disk). Shouldn't happen a lot, but if it happens, we would automatically recover. But you are right with not handling DCHECKs gracefully. So maybe just log an error and return false? https://codereview.chromium.org/2585263002/diff/80001/components/ntp_snippets... File components/ntp_snippets/category_rankers/click_based_category_ranker.cc (right): https://codereview.chromium.org/2585263002/diff/80001/components/ntp_snippets... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:161: void ClickBasedCategoryRanker::UpdateRanking(Category category) { On 2016/12/21 10:38:26, jkrcal wrote: > nit: I do not think this is a good abstraction / naming. This function is > correct only assuming that: > - the clicks for |category| have increased; > - the clicks for any other category have not changed. > Thus, it is directly bound to its only use case at the moment: > OnSuggestionOpened(). > > I suggest to rename it to AddClick(Category category) end incorporate lines 100 > - 105 in this function. :-) This was actually where we were coming from :-) What about simply renaming to UpdateRankingForCategory()? That way, the overall logic OnSuggestionOpened() is easy to follow -- it's 1) increment counter 2) update the ranking 3) store to prefs.
The CQ bit was checked by vitaliii@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...
tschumann@, I addressed your comments, PTAL. jkrcal@, feel free to have a look, but it is not mandatory :) https://codereview.chromium.org/2585263002/diff/40001/components/ntp_snippets... File components/ntp_snippets/category_rankers/click_based_category_ranker.cc (right): https://codereview.chromium.org/2585263002/diff/40001/components/ntp_snippets... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:32: const int kExtraClickPerPositionThresholdIncrease = 2; On 2016/12/21 12:32:28, tschumann wrote: > On 2016/12/21 10:38:26, jkrcal wrote: > > On 2016/12/21 09:25:01, vitaliii wrote: > > > On 2016/12/20 16:27:02, tschumann wrote: > > > > the constant name is misleading -- it's a multiplier / factor. > > > > I think we should introduce some better concept here. "ExtraClick" doesn't > > > > really have a lot of meaning. At some point we should also start thinking > > > about > > > > a "score" concept. Because after a while it's actually not comparable to > > > clicks > > > > anymore (time-decay and stuff). > > > > > > > > the extra-click's purpose is to stabilize the ranking. > > > > Just brain-storming here, maybe you like some of them: > > > > PassingTax > > > > PassingPenalty > > > > > > > > kIncreasedExtraClickThresholdCategoryCount would then become > > > > MaxPassingTaxRank > > > > > > > > and kExtraClickPerPositionThresholdIncrease could be PassingTaxFactor > (not > > > sure > > > > we need to mention the rank here -- i wouldn't use the term position; rank > > has > > > > pretty precise semantics when it comes to ranking :-). > > > > > > "Tax" implies decrease. > > > > > > I rewrote using "boundary width" and "top categories". > > > I like boundary width, because then a category is moved upwards only when it > > > crosses boundary of the previous category; before it is either behind or on > > the > > > boundary. > > > > I am not happy about "boundary width", I like "passing". > > What about "passing margin"? > > > > I do not feel strongly. If Tim is fine with "boundary width", I am also fine. > > boundary isn't too great but already better. > "tax means decrease": Well, tax means a certain way of punishment. So if you > want to pass, you also need to pay that tax. The metaphor doesn't work 100% as > you don't actually pay it; the score won't get reduced. But the overall image > gives the whole thing a sense. > > PassingMargin also sounds good. Maybe PassingGap ? Done. I took PassingMargin, because margin ("an amount by which something is won") fits well the intended meaning. https://codereview.chromium.org/2585263002/diff/40001/components/ntp_snippets... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:195: << " into dictionary."; On 2016/12/21 12:32:28, tschumann wrote: > On 2016/12/21 09:25:01, vitaliii wrote: > > On 2016/12/20 16:27:03, tschumann wrote: > > > shouldn't we return false in case of !success? > > > > DCHECKs should not be handled. > > Why do you think we will hit these DCHECKs? > > The only case I see is when we change the format of prefs data, but then this > > must be handled accordingly and DCHECKs will be removed. > > > > BTW, should I store some data version marker, so that we could parse it easily > > if we change the format? > > no, don't worry about versioning. There are other ways to deal with that (we > could also use a new key). > > One scenario is data corruption (we're storing it on disk). Shouldn't happen a > lot, but if it happens, we would automatically recover. > But you are right with not handling DCHECKs gracefully. So maybe just log an > error and return false? Done. LOG(DFATAL) and return false. https://codereview.chromium.org/2585263002/diff/80001/components/ntp_snippets... File components/ntp_snippets/category_rankers/click_based_category_ranker.cc (right): https://codereview.chromium.org/2585263002/diff/80001/components/ntp_snippets... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:36: // which is then combined with a top category position. On 2016/12/21 10:38:26, jkrcal wrote: > Did not understand at all. My suggestion: > > // The increase of boundary width for each top category compared to the next > category (e.g. the first top category has boundary width larger than the second > top category, the last top category has it larger than the first non-top > category). > const int kTopCategoryBoundaryWidthIcreasePerPosition = 2; // clicks > > This may be actually close to your original proposal, maybe it's better to agree > with Tim before any further renamings :) Done. https://codereview.chromium.org/2585263002/diff/80001/components/ntp_snippets... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:47: RestoreDefaultOrder(); On 2016/12/21 10:38:26, jkrcal wrote: > This does not address the situation if there is a correct order in prefs but we > introduce a new category. You will load the previous order correctly and the > ranker will never count clicks for the new category. > > If you want to leave it for a follow-up CL, just file a bug and add a TODO here. Good observation. I created a bug and added the TODO. https://codereview.chromium.org/2585263002/diff/80001/components/ntp_snippets... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:161: void ClickBasedCategoryRanker::UpdateRanking(Category category) { On 2016/12/21 10:38:26, jkrcal wrote: > nit: I do not think this is a good abstraction / naming. This function is > correct only assuming that: > - the clicks for |category| have increased; > - the clicks for any other category have not changed. > Thus, it is directly bound to its only use case at the moment: > OnSuggestionOpened(). > > I suggest to rename it to AddClick(Category category) end incorporate lines 100 > - 105 in this function. Done. I inlined the function, because now it included almost entire OnSuggestionOpened. https://codereview.chromium.org/2585263002/diff/80001/components/ntp_snippets... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:161: void ClickBasedCategoryRanker::UpdateRanking(Category category) { On 2016/12/21 12:32:28, tschumann wrote: > On 2016/12/21 10:38:26, jkrcal wrote: > > nit: I do not think this is a good abstraction / naming. This function is > > correct only assuming that: > > - the clicks for |category| have increased; > > - the clicks for any other category have not changed. > > Thus, it is directly bound to its only use case at the moment: > > OnSuggestionOpened(). > > > > I suggest to rename it to AddClick(Category category) end incorporate lines > 100 > > - 105 in this function. > > :-) This was actually where we were coming from :-) What about simply renaming > to UpdateRankingForCategory()? > > That way, the overall logic OnSuggestionOpened() is easy to follow -- it's 1) > increment counter 2) update the ranking 3) store to prefs. Ack. As jkrcal@ mentioned, this function assumes that click has happened right before the call, which is not reflected in the name at all. OnSuggestionOpened() is not large, so I do not think that inlining everything makes it hard to understand. https://codereview.chromium.org/2585263002/diff/80001/components/ntp_snippets... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:171: // It is indended to move only by one position at a time in order to avoid On 2016/12/21 10:38:26, jkrcal wrote: > nit: intended Done. https://codereview.chromium.org/2585263002/diff/80001/components/ntp_snippets... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:191: << "Failed to parse dismissed id from prefs param " On 2016/12/21 10:38:26, jkrcal wrote: > update the error message Done. https://codereview.chromium.org/2585263002/diff/80001/components/ntp_snippets... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:228: bool ClickBasedCategoryRanker::ContainsCategory(Category category) const { On 2016/12/21 10:38:26, jkrcal wrote: > nit: why not implementing using: > > return (FindCategory(category) != ordered_categories_.end()); Ack. FindCategory(category) cannot be const, because it returns a non const iterator (there is need to modify returned item). ConstainsCategory is const and there is no need to make it non const. https://codereview.chromium.org/2585263002/diff/80001/components/ntp_snippets... File components/ntp_snippets/category_rankers/click_based_category_ranker.h (right): https://codereview.chromium.org/2585263002/diff/80001/components/ntp_snippets... components/ntp_snippets/category_rankers/click_based_category_ranker.h:40: // category upwards. See .cc file for more details. On 2016/12/21 10:38:26, jkrcal wrote: > I would omit the second sentence (also below), this is kind of implied for every > function. Done.
The CQ bit was checked by vitaliii@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...
bauerb@chromium.org changed reviewers: + bauerb@chromium.org
browser_prefs LGTM https://codereview.chromium.org/2585263002/diff/40001/components/ntp_snippets... File components/ntp_snippets/category_rankers/click_based_category_ranker.cc (right): https://codereview.chromium.org/2585263002/diff/40001/components/ntp_snippets... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:195: << " into dictionary."; On 2016/12/21 13:16:31, vitaliii wrote: > On 2016/12/21 12:32:28, tschumann wrote: > > On 2016/12/21 09:25:01, vitaliii wrote: > > > On 2016/12/20 16:27:03, tschumann wrote: > > > > shouldn't we return false in case of !success? > > > > > > DCHECKs should not be handled. > > > Why do you think we will hit these DCHECKs? > > > The only case I see is when we change the format of prefs data, but then > this > > > must be handled accordingly and DCHECKs will be removed. > > > > > > BTW, should I store some data version marker, so that we could parse it > easily > > > if we change the format? > > > > no, don't worry about versioning. There are other ways to deal with that (we > > could also use a new key). > > > > One scenario is data corruption (we're storing it on disk). Shouldn't happen a > > lot, but if it happens, we would automatically recover. > > But you are right with not handling DCHECKs gracefully. So maybe just log an > > error and return false? > > Done. > > LOG(DFATAL) and return false. Maybe add a comment why this is DFATAL? That is somewhat uncommon. https://codereview.chromium.org/2585263002/diff/120001/components/ntp_snippet... File components/ntp_snippets/category_rankers/click_based_category_ranker.cc (right): https://codereview.chromium.org/2585263002/diff/120001/components/ntp_snippet... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:26: // margin of the previous category position more clicks. It might be worth to move these explanations to the header? https://codereview.chromium.org/2585263002/diff/120001/components/ntp_snippet... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:38: // value than the first non-top category). Just for my understanding: This means that the very first category needs to have 11 more clicks than the second one, which needs to have 9 more clicks than the third one, which needs to have 7 more clicks than the fourth one, and from the fourth one on it's 5? https://codereview.chromium.org/2585263002/diff/120001/components/ntp_snippet... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:106: ++current->clicks; Nit: I don't think using the preincrement operator here actually saves a measurable amount of time, and it reads a bit more awkward to me than the postincrement form. https://codereview.chromium.org/2585263002/diff/120001/components/ntp_snippet... File components/ntp_snippets/category_rankers/click_based_category_ranker.h (right): https://codereview.chromium.org/2585263002/diff/120001/components/ntp_snippet... components/ntp_snippets/category_rankers/click_based_category_ranker.h:40: // category upwards. You could a comment that these are public just for testing.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
tschumann@ comments from offline discussion. https://codereview.chromium.org/2585263002/diff/120001/components/ntp_snippet... File components/ntp_snippets/category_rankers/click_based_category_ranker.cc (right): https://codereview.chromium.org/2585263002/diff/120001/components/ntp_snippet... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:27: const int kPassingMargin = 5; // clicks In offline discussion tschumann@ suggested: no units. https://codereview.chromium.org/2585263002/diff/120001/components/ntp_snippet... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:33: const int kTopCategoriesCount = 3; In offline discussion tschumann@ suggested: kNumTopCategoriesWithExtraMargin https://codereview.chromium.org/2585263002/diff/120001/components/ntp_snippet... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:39: const int kTopCategoryPassingIncreasePerPosition = 2; // clicks / position In offline discussion tschumann@ suggested: kExtraPassingMargin = 2; and no units.
The CQ bit was checked by vitaliii@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...
Hi tschumann@, I addressed all the comments, please PTAL. https://codereview.chromium.org/2585263002/diff/120001/components/ntp_snippet... File components/ntp_snippets/category_rankers/click_based_category_ranker.cc (right): https://codereview.chromium.org/2585263002/diff/120001/components/ntp_snippet... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:26: // margin of the previous category position more clicks. On 2016/12/21 15:13:12, Bernhard Bauer wrote: > It might be worth to move these explanations to the header? Acknowledged. I like constants to follow related comments here. https://codereview.chromium.org/2585263002/diff/120001/components/ntp_snippet... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:27: const int kPassingMargin = 5; // clicks On 2016/12/22 07:13:30, vitaliii wrote: > In offline discussion tschumann@ suggested: no units. Done. https://codereview.chromium.org/2585263002/diff/120001/components/ntp_snippet... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:33: const int kTopCategoriesCount = 3; On 2016/12/22 07:13:30, vitaliii wrote: > In offline discussion tschumann@ suggested: kNumTopCategoriesWithExtraMargin Done. https://codereview.chromium.org/2585263002/diff/120001/components/ntp_snippet... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:38: // value than the first non-top category). On 2016/12/21 15:13:12, Bernhard Bauer wrote: > Just for my understanding: This means that the very first category needs to have > 11 more clicks than the second one, which needs to have 9 more clicks than the > third one, which needs to have 7 more clicks than the fourth one, and from the > fourth one on it's 5? True. https://codereview.chromium.org/2585263002/diff/120001/components/ntp_snippet... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:39: const int kTopCategoryPassingIncreasePerPosition = 2; // clicks / position On 2016/12/22 07:13:30, vitaliii wrote: > In offline discussion tschumann@ suggested: kExtraPassingMargin = 2; > and no units. Done. https://codereview.chromium.org/2585263002/diff/120001/components/ntp_snippet... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:106: ++current->clicks; On 2016/12/21 15:13:12, Bernhard Bauer wrote: > Nit: I don't think using the preincrement operator here actually saves a > measurable amount of time, and it reads a bit more awkward to me than the > postincrement form. Done. https://codereview.chromium.org/2585263002/diff/120001/components/ntp_snippet... File components/ntp_snippets/category_rankers/click_based_category_ranker.h (right): https://codereview.chromium.org/2585263002/diff/120001/components/ntp_snippet... components/ntp_snippets/category_rankers/click_based_category_ranker.h:40: // category upwards. On 2016/12/21 15:13:12, Bernhard Bauer wrote: > You could a comment that these are public just for testing. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by vitaliii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jkrcal@chromium.org, bauerb@chromium.org Link to the patchset: https://codereview.chromium.org/2585263002/#ps160001 (title: "tschumann@ & bauerb@ 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": 160001, "attempt_start_ts": 1482398559826230,
"parent_rev": "665bb72185f0210c885e845a2544779834fb62b3", "commit_rev":
"562ec190e686ba418378390a42f0d3d10efc19c6"}
Message was sent while issue was closed.
Description was changed from ========== [NTP::SectionOrder] First version of click based category ranker. This CL adds an implementation of CategoryRanker based on clicks. BUG=674851 ========== to ========== [NTP::SectionOrder] First version of click based category ranker. This CL adds an implementation of CategoryRanker based on clicks. BUG=674851 Review-Url: https://codereview.chromium.org/2585263002 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== [NTP::SectionOrder] First version of click based category ranker. This CL adds an implementation of CategoryRanker based on clicks. BUG=674851 Review-Url: https://codereview.chromium.org/2585263002 ========== to ========== [NTP::SectionOrder] First version of click based category ranker. This CL adds an implementation of CategoryRanker based on clicks. BUG=674851 Committed: https://crrev.com/9f8aa5f8fc0acdbb5f473687f547edcc342df687 Cr-Commit-Position: refs/heads/master@{#440369} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/9f8aa5f8fc0acdbb5f473687f547edcc342df687 Cr-Commit-Position: refs/heads/master@{#440369} |
