Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1847713002:
Add histograms to measure CLD2 language detection as well as the accuracy of the chosen detection. (Closed)
DescriptionAdd histograms to measure CLD2 language detection as well as the accuracy of the chosen detection.
BUG=596537
Committed: https://crrev.com/eb4ffca7b23f6f8d1a0af549bfc18e31a3a0e331
Cr-Commit-Position: refs/heads/master@{#384575}
Patch Set 1 #Patch Set 2 : Remove the summary language bugfix #
Total comments: 5
Patch Set 3 : asvitkine comment #
Messages
Total messages: 21 (8 generated)
Description was changed from ========== Add histograms to measure CLD2 language detection as well as the accuracy of the chosen detection. BUG=596537 ========== to ========== Add histograms to measure CLD2 language detection as well as the accuracy of the chosen detection. This also includes a follow up to https://codereview.chromium.org/1263613002 which removed the summary_language usage. Currently it looks like there's a small issue, where if the language3[0] is unknown - we only are checking the summary language, which (based on my skimming of CLD2) can be language[1] if language3[0] is bad in some way. Adjusted to check language3[0] only and ignore the summary return BUG=596537 ==========
rkaplow@chromium.org changed reviewers: + andrewhayden@chromium.org, asvitkine@chromium.org
These should be split up. The change to use the [0] language could conceivably cause some minor (unimportant) regressions and I'd hate to see the histogram stuff rolled back along with it in that event. Please separate these; aside from that, ELL-GEE-TEE-EMM.
On 2016/03/31 01:37:06, Andrew Hayden (chromium.org) wrote: > These should be split up. The change to use the [0] language could conceivably > cause some minor (unimportant) regressions and I'd hate to see the histogram > stuff rolled back along with it in that event. Please separate these; aside from > that, ELL-GEE-TEE-EMM. Ok, updated without the summary lang bugfix. Will send another with it
https://codereview.chromium.org/1847713002/diff/20001/components/translate/co... File components/translate/core/language_detection/language_detection_util.cc (right): https://codereview.chromium.org/1847713002/diff/20001/components/translate/co... components/translate/core/language_detection/language_detection_util.cc:141: cld_language, CLD2::NUM_LANGUAGES); Does CLD guarantee that it's language enums won't be renumbered in a new version of the library? https://codereview.chromium.org/1847713002/diff/20001/components/translate/co... components/translate/core/language_detection/language_detection_util.cc:142: if (is_valid_language) { Nit: No {}'s https://codereview.chromium.org/1847713002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1847713002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:53833: +<histogram name="Translate.CLD2LanguageDetected" enum="CLD2LanguageCode"> Nit: How about adding an intermediate . to the naming scheme - e.g. Translate.CLD2.LanguageDetected. Makes it a bit more readable.
On 2016/03/31 15:10:22, Alexei Svitkine wrote: > https://codereview.chromium.org/1847713002/diff/20001/components/translate/co... > File components/translate/core/language_detection/language_detection_util.cc > (right): > > https://codereview.chromium.org/1847713002/diff/20001/components/translate/co... > components/translate/core/language_detection/language_detection_util.cc:141: > cld_language, CLD2::NUM_LANGUAGES); > Does CLD guarantee that it's language enums won't be renumbered in a new version > of the library? > > https://codereview.chromium.org/1847713002/diff/20001/components/translate/co... > components/translate/core/language_detection/language_detection_util.cc:142: if > (is_valid_language) { > Nit: No {}'s > > https://codereview.chromium.org/1847713002/diff/20001/tools/metrics/histogram... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/1847713002/diff/20001/tools/metrics/histogram... > tools/metrics/histograms/histograms.xml:53833: +<histogram > name="Translate.CLD2LanguageDetected" enum="CLD2LanguageCode"> > Nit: How about adding an intermediate . to the naming scheme - e.g. > Translate.CLD2.LanguageDetected. Makes it a bit more readable. Done on metric rename
https://codereview.chromium.org/1847713002/diff/20001/components/translate/co... File components/translate/core/language_detection/language_detection_util.cc (right): https://codereview.chromium.org/1847713002/diff/20001/components/translate/co... components/translate/core/language_detection/language_detection_util.cc:141: cld_language, CLD2::NUM_LANGUAGES); On 2016/03/31 15:10:21, Alexei Svitkine wrote: > Does CLD guarantee that it's language enums won't be renumbered in a new version > of the library? I suspect only the unused space (i.e. x_nnn) would be replaced with actual languages. It seems pretty unlikely to me they would do reordering (also doesn't look like there's much more development) Not sure what you would suggest - I could add a comment here to warn that a new histogram would need to be added if the enum is reordered but that seems overkill. https://codereview.chromium.org/1847713002/diff/20001/components/translate/co... components/translate/core/language_detection/language_detection_util.cc:142: if (is_valid_language) { On 2016/03/31 15:10:22, Alexei Svitkine wrote: > Nit: No {}'s Done.
Description was changed from ========== Add histograms to measure CLD2 language detection as well as the accuracy of the chosen detection. This also includes a follow up to https://codereview.chromium.org/1263613002 which removed the summary_language usage. Currently it looks like there's a small issue, where if the language3[0] is unknown - we only are checking the summary language, which (based on my skimming of CLD2) can be language[1] if language3[0] is bad in some way. Adjusted to check language3[0] only and ignore the summary return BUG=596537 ========== to ========== Add histograms to measure CLD2 language detection as well as the accuracy of the chosen detection. BUG=596537 ==========
LGTM. Thanks!
lgtm
The CQ bit was checked by rkaplow@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1847713002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1847713002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by rkaplow@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1847713002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1847713002/40001
Message was sent while issue was closed.
Description was changed from ========== Add histograms to measure CLD2 language detection as well as the accuracy of the chosen detection. BUG=596537 ========== to ========== Add histograms to measure CLD2 language detection as well as the accuracy of the chosen detection. BUG=596537 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Add histograms to measure CLD2 language detection as well as the accuracy of the chosen detection. BUG=596537 ========== to ========== Add histograms to measure CLD2 language detection as well as the accuracy of the chosen detection. BUG=596537 Committed: https://crrev.com/eb4ffca7b23f6f8d1a0af549bfc18e31a3a0e331 Cr-Commit-Position: refs/heads/master@{#384575} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/eb4ffca7b23f6f8d1a0af549bfc18e31a3a0e331 Cr-Commit-Position: refs/heads/master@{#384575} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
