Record scores of distillability and long article in UMA

Record scores of distillability and long article in UMA

BUG=617769
Committed: https://crrev.com/9e7576cc96261045a9a72ab14a57e7204f09f415
Cr-Commit-Position: refs/heads/master@{#398483}

[wychen, 2016-06-06 21:43:41]

wychen@chromium.org changed reviewers:
+ mdjones@chromium.org, rkaplow@chromium.org

[wychen, 2016-06-06 21:43:41]

PTAL

[wychen, 2016-06-06 21:46:02]

Description was changed from

==========
Record scores of distillability and long article in UMA

BUG=
==========

to

==========
Record scores of distillability and long article in UMA

BUG=617769
==========

[rkaplow, 2016-06-07 20:51:12]

https://codereview.chromium.org/2021643002/diff/20001/components/dom_distille...
File components/dom_distiller/content/renderer/distillability_agent.cc (right):

https://codereview.chromium.org/2021643002/diff/20001/components/dom_distille...
components/dom_distiller/content/renderer/distillability_agent.cc:111: if
(distillable) {
this is confusing me, since the above section says Distillability metrics.

https://codereview.chromium.org/2021643002/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2021643002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:9944: +<histogram
name="DomDistiller.DistillabilityScoreNMF.Negative" units="0.01">
can you improve this unit? Maybe put "score", and then in the description
mention it's multiplied by 100? Our tools won't make this look very
understandable I think.

[wychen, 2016-06-07 21:27:33]

https://codereview.chromium.org/2021643002/diff/20001/components/dom_distille...
File components/dom_distiller/content/renderer/distillability_agent.cc (right):

https://codereview.chromium.org/2021643002/diff/20001/components/dom_distille...
components/dom_distiller/content/renderer/distillability_agent.cc:111: if
(distillable) {
On 2016/06/07 20:51:12, rkaplow wrote:
> this is confusing me, since the above section says Distillability metrics.

The long-page score is only relevant if the first model (|detector|) says yes,
since it is trained this way.
Therefore, we only collect the distribution in this case.

https://codereview.chromium.org/2021643002/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2021643002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:9944: +<histogram
name="DomDistiller.DistillabilityScoreNMF.Negative" units="0.01">
On 2016/06/07 20:51:12, rkaplow wrote:
> can you improve this unit? Maybe put "score", and then in the description
> mention it's multiplied by 100? Our tools won't make this look very
> understandable I think.

Done.

[rkaplow, 2016-06-07 21:37:27]

On 2016/06/07 21:27:33, wychen wrote:
>
https://codereview.chromium.org/2021643002/diff/20001/components/dom_distille...
> File components/dom_distiller/content/renderer/distillability_agent.cc
(right):
> 
>
https://codereview.chromium.org/2021643002/diff/20001/components/dom_distille...
> components/dom_distiller/content/renderer/distillability_agent.cc:111: if
> (distillable) {
> On 2016/06/07 20:51:12, rkaplow wrote:
> > this is confusing me, since the above section says Distillability metrics.
> 
> The long-page score is only relevant if the first model (|detector|) says yes,
> since it is trained this way.
> Therefore, we only collect the distribution in this case.

I would add a comment to that effect since this isn;t obvious from the code.
> 
>
https://codereview.chromium.org/2021643002/diff/20001/tools/metrics/histogram...
> File tools/metrics/histograms/histograms.xml (right):
> 
>
https://codereview.chromium.org/2021643002/diff/20001/tools/metrics/histogram...
> tools/metrics/histograms/histograms.xml:9944: +<histogram
> name="DomDistiller.DistillabilityScoreNMF.Negative" units="0.01">
> On 2016/06/07 20:51:12, rkaplow wrote:
> > can you improve this unit? Maybe put "score", and then in the description
> > mention it's multiplied by 100? Our tools won't make this look very
> > understandable I think.
> 
> Done.

[rkaplow, 2016-06-07 21:37:32]

lgtm

[mdjones, 2016-06-07 21:54:16]

lgtm

[wychen, 2016-06-07 22:12:32]

On 2016/06/07 21:37:27, rkaplow wrote:
> On 2016/06/07 21:27:33, wychen wrote:
> >
>
https://codereview.chromium.org/2021643002/diff/20001/components/dom_distille...
> > File components/dom_distiller/content/renderer/distillability_agent.cc
> (right):
> > 
> >
>
https://codereview.chromium.org/2021643002/diff/20001/components/dom_distille...
> > components/dom_distiller/content/renderer/distillability_agent.cc:111: if
> > (distillable) {
> > On 2016/06/07 20:51:12, rkaplow wrote:
> > > this is confusing me, since the above section says Distillability metrics.
> > 
> > The long-page score is only relevant if the first model (|detector|) says
yes,
> > since it is trained this way.
> > Therefore, we only collect the distribution in this case.
> 
> I would add a comment to that effect since this isn;t obvious from the code.

Done

[wychen, 2016-06-08 03:34:53]

The CQ bit was checked by wychen@chromium.org

[wychen, 2016-06-08 03:34:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
mdjones@chromium.org, rkaplow@chromium.org
Link to the patchset: https://codereview.chromium.org/2021643002/#ps60001
(title: "add comments")

[commit-bot: I haz the power, 2016-06-08 03:35:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2021643002/60001

[commit-bot: I haz the power, 2016-06-08 04:48:24]

Description was changed from

==========
Record scores of distillability and long article in UMA

BUG=617769
==========

to

==========
Record scores of distillability and long article in UMA

BUG=617769
==========

[commit-bot: I haz the power, 2016-06-08 04:48:25]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-08 04:49:27]

Description was changed from

==========
Record scores of distillability and long article in UMA

BUG=617769
==========

to

==========
Record scores of distillability and long article in UMA

BUG=617769

Committed: https://crrev.com/9e7576cc96261045a9a72ab14a57e7204f09f415
Cr-Commit-Position: refs/heads/master@{#398483}
==========

[commit-bot: I haz the power, 2016-06-08 04:49:28]

Patchset 4 (id:??) landed as
https://crrev.com/9e7576cc96261045a9a72ab14a57e7204f09f415
Cr-Commit-Position: refs/heads/master@{#398483}