[NTP::SectionOrder] Make ClickBasedRanker move dismissed sections down.

[NTP::SectionOrder] Make ClickBasedRanker move dismissed sections down.

Move dissmised sections 1 position down in ClickBasedRanker. Dismissing a
section is a strong negative signal, thus, category rankers can react to it.

BUG=677781
Committed: https://crrev.com/3d9423e34805ed9a6046d44e6ce144a39b16f603
Cr-Commit-Position: refs/heads/master@{#441140}

[vitaliii, 2017-01-02 10:48:08]

The CQ bit was checked by vitaliii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-02 10:48:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-02 12:23:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-02 12:23:52]

Dry run: This issue passed the CQ dry run.

[vitaliii, 2017-01-02 12:29:50]

vitaliii@chromium.org changed reviewers:
+ jkrcal@chromium.org

[vitaliii, 2017-01-02 12:29:50]

Hi jkrcal@,

Could you have a look?

[jkrcal, 2017-01-02 12:49:47]

https://codereview.chromium.org/2610553002/diff/1/components/ntp_snippets/cat...
File components/ntp_snippets/category_rankers/click_based_category_ranker.cc
(right):

https://codereview.chromium.org/2610553002/diff/1/components/ntp_snippets/cat...
components/ntp_snippets/category_rankers/click_based_category_ranker.cc:42:
const int kDismissedCategoryPenalty = 2;
Why "2"?

Are we sure dismissing a category is meant as a strong negative signal by the
user?
(Isn't it "I dismissed all because it is not useful at the moment and I
dismissed the category because an empty category is not useful"? Do we have any
observations from UX research?) 

Do we know how often people dismiss a category?

https://codereview.chromium.org/2610553002/diff/1/components/ntp_snippets/cat...
File
components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc
(right):

https://codereview.chromium.org/2610553002/diff/1/components/ntp_snippets/cat...
components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc:245:
ShouldMovePenultimateCategoryDownWhenDismissed) {
nit: I need to admit I had to look up "Penultimate" in the dictionary. Wouldn't
"SecondLast" be more accessible to non-native speakers?

https://codereview.chromium.org/2610553002/diff/1/components/ntp_snippets/cat...
components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc:252:
}
I do not understand the difference of what is this test doing differently from
the previous test (at least the naming does not help).

Shouldn't there be a test with kDismissedCategoryPenalty+1 categories?

https://codereview.chromium.org/2610553002/diff/1/components/ntp_snippets/cat...
components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc:274:
NotifyOnCategoryDismissed(second);
nit: Add a comment "// This should reduce the click counter back to 0."

[vitaliii, 2017-01-03 07:32:05]

The CQ bit was checked by vitaliii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-03 07:32:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vitaliii, 2017-01-03 07:32:23]

Hi jkrcal@,

I addressed your comments,
please have a look.

https://codereview.chromium.org/2610553002/diff/1/components/ntp_snippets/cat...
File components/ntp_snippets/category_rankers/click_based_category_ranker.cc
(right):

https://codereview.chromium.org/2610553002/diff/1/components/ntp_snippets/cat...
components/ntp_snippets/category_rankers/click_based_category_ranker.cc:42:
const int kDismissedCategoryPenalty = 2;
On 2017/01/02 12:49:47, jkrcal wrote:
> Why "2"?

With "1" if one dismisses two categories in a row, they end up with the same
order as it was before (though reduced click counts).

> 
> Are we sure dismissing a category is meant as a strong negative signal by the
> user?

No, but currently the category dismissal is not functional at all in my view.
I am not aware of any reason to dismiss a category at all.

> (Isn't it "I dismissed all because it is not useful at the moment and I
> dismissed the category because an empty category is not useful"? Do we have
any
> observations from UX research?) 

In my view - no, empty category won't be shown on new NTPs anyway.
Currently, I am not aware of what dismissing a section does.
In terms of new NTPs the behavior is exactly the same as just dismissing all
suggestions.

As for the UX research I am not aware of any.

But since people seem to know that dismissing means "I do not like / want to see
this", it seems reasonable to assume that dismissing a section has similar
negative meaning.

> Do we know how often people dismiss a category?
Not often at all.

https://codereview.chromium.org/2610553002/diff/1/components/ntp_snippets/cat...
File
components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc
(right):

https://codereview.chromium.org/2610553002/diff/1/components/ntp_snippets/cat...
components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc:245:
ShouldMovePenultimateCategoryDownWhenDismissed) {
On 2017/01/02 12:49:47, jkrcal wrote:
> nit: I need to admit I had to look up "Penultimate" in the dictionary.
Wouldn't
> "SecondLast" be more accessible to non-native speakers?

Done. 

"SecondToLast".

https://codereview.chromium.org/2610553002/diff/1/components/ntp_snippets/cat...
components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc:252:
}
On 2017/01/02 12:49:47, jkrcal wrote:
> I do not understand the difference of what is this test doing differently from
> the previous test (at least the naming does not help).

This test dismisses second to last category, while in the previous test it is
one of the top categories. I added comments to make this explicit.

> Shouldn't there be a test with kDismissedCategoryPenalty+1 categories?
 
Done.

https://codereview.chromium.org/2610553002/diff/1/components/ntp_snippets/cat...
components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc:274:
NotifyOnCategoryDismissed(second);
On 2017/01/02 12:49:47, jkrcal wrote:
> nit: Add a comment "// This should reduce the click counter back to 0."

Done.

[commit-bot: I haz the power, 2017-01-03 08:26:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-03 08:26:06]

Dry run: This issue passed the CQ dry run.

[jkrcal, 2017-01-03 08:46:19]

https://codereview.chromium.org/2610553002/diff/1/components/ntp_snippets/cat...
File components/ntp_snippets/category_rankers/click_based_category_ranker.cc
(right):

https://codereview.chromium.org/2610553002/diff/1/components/ntp_snippets/cat...
components/ntp_snippets/category_rankers/click_based_category_ranker.cc:42:
const int kDismissedCategoryPenalty = 2;
On 2017/01/03 07:32:22, vitaliii wrote:
> On 2017/01/02 12:49:47, jkrcal wrote:
> > Why "2"?
> 
> With "1" if one dismisses two categories in a row, they end up with the same
> order as it was before (though reduced click counts).

Good point. I am not sure how common a use case this is.

> > Are we sure dismissing a category is meant as a strong negative signal by
the
> > user?
> 
> No, but currently the category dismissal is not functional at all in my view.
> I am not aware of any reason to dismiss a category at all.

Do I get it right that you are making it (at least partially) functional with
this CL?

> > (Isn't it "I dismissed all because it is not useful at the moment and I
> > dismissed the category because an empty category is not useful"? Do we have
> any
> > observations from UX research?) 
> 
> In my view - no, empty category won't be shown on new NTPs anyway.
> Currently, I am not aware of what dismissing a section does.
> In terms of new NTPs the behavior is exactly the same as just dismissing all
> suggestions.
> 
> As for the UX research I am not aware of any.
> 
> But since people seem to know that dismissing means "I do not like / want to
see
> this", it seems reasonable to assume that dismissing a section has similar
> negative meaning.

I am not sure. By looking at the data it seems to me that dismissing a category
can also mean "cleaning up".
Cleaning up a category that can generally be useful. Hard to distinguish from
"never show this again", though.

> > Do we know how often people dismiss a category?
> Not often at all.

I strongly advocate for decreasing the penalty to 1. Another approach would be
only decreasing click count by a constant (such as the passing margin) and swap
one pair of needed (as with increasing the click count by one).

https://codereview.chromium.org/2610553002/diff/1/components/ntp_snippets/cat...
File
components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc
(right):

https://codereview.chromium.org/2610553002/diff/1/components/ntp_snippets/cat...
components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc:245:
ShouldMovePenultimateCategoryDownWhenDismissed) {
On 2017/01/03 07:32:22, vitaliii wrote:
> On 2017/01/02 12:49:47, jkrcal wrote:
> > nit: I need to admit I had to look up "Penultimate" in the dictionary.
> Wouldn't
> > "SecondLast" be more accessible to non-native speakers?
> 
> Done. 
> 
> "SecondToLast".

Even better, thanks!

https://codereview.chromium.org/2610553002/diff/40001/components/ntp_snippets...
File components/ntp_snippets/category_rankers/click_based_category_ranker.h
(right):

https://codereview.chromium.org/2610553002/diff/40001/components/ntp_snippets...
components/ntp_snippets/category_rankers/click_based_category_ranker.h:48: //
Returns number of positions by which a dismissed category is downgraded.
nit: For testing only? Why don't you reflect "ForTesting" in the naming of all
the three functions?

[vitaliii, 2017-01-03 15:56:17]

Hi jkrcal@,

I addressed your comments,

could you have a look?

https://codereview.chromium.org/2610553002/diff/1/components/ntp_snippets/cat...
File components/ntp_snippets/category_rankers/click_based_category_ranker.cc
(right):

https://codereview.chromium.org/2610553002/diff/1/components/ntp_snippets/cat...
components/ntp_snippets/category_rankers/click_based_category_ranker.cc:42:
const int kDismissedCategoryPenalty = 2;
On 2017/01/03 08:46:19, jkrcal wrote:
> On 2017/01/03 07:32:22, vitaliii wrote:
> > On 2017/01/02 12:49:47, jkrcal wrote:
> > > Why "2"?
> > 
> > With "1" if one dismisses two categories in a row, they end up with the same
> > order as it was before (though reduced click counts).
> 
> Good point. I am not sure how common a use case this is.
> 
> > > Are we sure dismissing a category is meant as a strong negative signal by
> the
> > > user?
> > 
> > No, but currently the category dismissal is not functional at all in my
view.
> > I am not aware of any reason to dismiss a category at all.
> 
> Do I get it right that you are making it (at least partially) functional with
> this CL?

I am not sure what you mean by "(at least partially) functional", but I can say
that after this CL I will know a reason to dismiss a category :)

> > > (Isn't it "I dismissed all because it is not useful at the moment and I
> > > dismissed the category because an empty category is not useful"? Do we
have
> > any
> > > observations from UX research?) 
> > 
> > In my view - no, empty category won't be shown on new NTPs anyway.
> > Currently, I am not aware of what dismissing a section does.
> > In terms of new NTPs the behavior is exactly the same as just dismissing all
> > suggestions.
> > 
> > As for the UX research I am not aware of any.
> > 
> > But since people seem to know that dismissing means "I do not like / want to
> see
> > this", it seems reasonable to assume that dismissing a section has similar
> > negative meaning.
> 
> I am not sure. By looking at the data it seems to me that dismissing a
category
> can also mean "cleaning up".
> Cleaning up a category that can generally be useful. Hard to distinguish from
> "never show this again", though.

I do not like "cleaning up" use case, because there is no point in it.
It has no effect on new NTPs.

> > > Do we know how often people dismiss a category?
> > Not often at all.
> 
> I strongly advocate for decreasing the penalty to 1. Another approach would be
> only decreasing click count by a constant (such as the passing margin) and
swap
> one pair of needed (as with increasing the click count by one).

Done.

Reduced to 1.

https://codereview.chromium.org/2610553002/diff/1/components/ntp_snippets/cat...
File
components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc
(right):

https://codereview.chromium.org/2610553002/diff/1/components/ntp_snippets/cat...
components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc:245:
ShouldMovePenultimateCategoryDownWhenDismissed) {
On 2017/01/03 08:46:19, jkrcal wrote:
> On 2017/01/03 07:32:22, vitaliii wrote:
> > On 2017/01/02 12:49:47, jkrcal wrote:
> > > nit: I need to admit I had to look up "Penultimate" in the dictionary.
> > Wouldn't
> > > "SecondLast" be more accessible to non-native speakers?
> > 
> > Done. 
> > 
> > "SecondToLast".
> 
> Even better, thanks!

Ack, np!

https://codereview.chromium.org/2610553002/diff/40001/components/ntp_snippets...
File components/ntp_snippets/category_rankers/click_based_category_ranker.h
(right):

https://codereview.chromium.org/2610553002/diff/40001/components/ntp_snippets...
components/ntp_snippets/category_rankers/click_based_category_ranker.h:48: //
Returns number of positions by which a dismissed category is downgraded.
On 2017/01/03 08:46:19, jkrcal wrote:
> nit: For testing only? Why don't you reflect "ForTesting" in the naming of all
> the three functions?

This was raised already and we agreed not to have "ForTesting".
I don't remember how it went exactly, but in the end I could not test properly
without the functions above.

[jkrcal, 2017-01-03 16:04:39]

lgtm

https://codereview.chromium.org/2610553002/diff/40001/components/ntp_snippets...
File components/ntp_snippets/category_rankers/click_based_category_ranker.h
(right):

https://codereview.chromium.org/2610553002/diff/40001/components/ntp_snippets...
components/ntp_snippets/category_rankers/click_based_category_ranker.h:48: //
Returns number of positions by which a dismissed category is downgraded.
On 2017/01/03 15:56:17, vitaliii wrote:
> On 2017/01/03 08:46:19, jkrcal wrote:
> > nit: For testing only? Why don't you reflect "ForTesting" in the naming of
all
> > the three functions?
> 
> This was raised already and we agreed not to have "ForTesting".
> I don't remember how it went exactly, but in the end I could not test properly
> without the functions above.

Okay, I am looking forward to a testing session to get it clarified :)

Could you please add "For testing only." to the comment?

[vitaliii, 2017-01-03 16:18:35]

Hi jkrcal@,

I addressed your comment, no need to look.

https://codereview.chromium.org/2610553002/diff/40001/components/ntp_snippets...
File components/ntp_snippets/category_rankers/click_based_category_ranker.h
(right):

https://codereview.chromium.org/2610553002/diff/40001/components/ntp_snippets...
components/ntp_snippets/category_rankers/click_based_category_ranker.h:48: //
Returns number of positions by which a dismissed category is downgraded.
On 2017/01/03 16:04:38, jkrcal wrote:
> On 2017/01/03 15:56:17, vitaliii wrote:
> > On 2017/01/03 08:46:19, jkrcal wrote:
> > > nit: For testing only? Why don't you reflect "ForTesting" in the naming of
> all
> > > the three functions?
> > 
> > This was raised already and we agreed not to have "ForTesting".
> > I don't remember how it went exactly, but in the end I could not test
properly
> > without the functions above.
> 
> Okay, I am looking forward to a testing session to get it clarified :)
> 
> Could you please add "For testing only." to the comment?

Done.

Me too.

[vitaliii, 2017-01-03 16:18:42]

The CQ bit was checked by vitaliii@chromium.org

[vitaliii, 2017-01-03 16:18:42]

The patchset sent to the CQ was uploaded after l-g-t-m from jkrcal@chromium.org
Link to the patchset: https://codereview.chromium.org/2610553002/#ps80001
(title: "jkrcal@ comment.")

[commit-bot: I haz the power, 2017-01-03 16:18:57]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-03 17:08:17]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1483460322519970,
"parent_rev": "ad7e1bfd17cec009c50ce0b7b94a4842bbca6e8d", "commit_rev":
"694bf8a89fd6d85890355454c8736bcb27a5317f"}

[commit-bot: I haz the power, 2017-01-03 17:08:43]

Description was changed from

==========
[NTP::SectionOrder] Make ClickBasedRanker move dismissed sections down.

Move dissmised sections 2 positions down in ClickBasedRanker. Dismissing a
section is a strong negative signal, thus, category rankers can react to it.

BUG=677781
==========

to

==========
[NTP::SectionOrder] Make ClickBasedRanker move dismissed sections down.

Move dissmised sections 2 positions down in ClickBasedRanker. Dismissing a
section is a strong negative signal, thus, category rankers can react to it.

BUG=677781

Review-Url: https://codereview.chromium.org/2610553002
==========

[commit-bot: I haz the power, 2017-01-03 17:08:44]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2017-01-03 17:10:31]

Description was changed from

==========
[NTP::SectionOrder] Make ClickBasedRanker move dismissed sections down.

Move dissmised sections 2 positions down in ClickBasedRanker. Dismissing a
section is a strong negative signal, thus, category rankers can react to it.

BUG=677781

Review-Url: https://codereview.chromium.org/2610553002
==========

to

==========
[NTP::SectionOrder] Make ClickBasedRanker move dismissed sections down.

Move dissmised sections 2 positions down in ClickBasedRanker. Dismissing a
section is a strong negative signal, thus, category rankers can react to it.

BUG=677781

Committed: https://crrev.com/3d9423e34805ed9a6046d44e6ce144a39b16f603
Cr-Commit-Position: refs/heads/master@{#441140}
==========

[commit-bot: I haz the power, 2017-01-03 17:10:32]

Patchset 5 (id:??) landed as
https://crrev.com/3d9423e34805ed9a6046d44e6ce144a39b16f603
Cr-Commit-Position: refs/heads/master@{#441140}

[vitaliii, 2017-01-03 17:30:38]

Description was changed from

==========
[NTP::SectionOrder] Make ClickBasedRanker move dismissed sections down.

Move dissmised sections 2 positions down in ClickBasedRanker. Dismissing a
section is a strong negative signal, thus, category rankers can react to it.

BUG=677781

Committed: https://crrev.com/3d9423e34805ed9a6046d44e6ce144a39b16f603
Cr-Commit-Position: refs/heads/master@{#441140}
==========

to

==========
[NTP::SectionOrder] Make ClickBasedRanker move dismissed sections down.

Move dissmised sections 1 position down in ClickBasedRanker. Dismissing a
section is a strong negative signal, thus, category rankers can react to it.

BUG=677781

Committed: https://crrev.com/3d9423e34805ed9a6046d44e6ce144a39b16f603
Cr-Commit-Position: refs/heads/master@{#441140}
==========