Fix data loss in Segmentation model execution metrics [chromium/src : main]
Alexis Wu (Gerrit)
Alexis Wu voted and added 1 comment![]( "Open in Gerrit")
Votes added by Alexis Wu
| Commit-Queue | +1 |
1 comment
Related details
- Salvador Guerrero Ramos
- Tommy Nyquist
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tommy Nyquist (Gerrit)
Tommy Nyquist voted and added 3 comments![]( "Open in Gerrit")
Votes added by Tommy Nyquist
| Code-Review | +1 |
3 comments
base::UmaHistogramPercentage(histogram_name + ".Raw", result[i]);Since `result[i]` is a float (from the `ModelProvider::Response` vector), passing it directly to `UmaHistogramPercentage` (which takes an integer) relies on implicit C++ truncation. This means a value like 0.9 would be recorded as 0 rather than 1, and 79.9 would become 79.
To avoid off-by-one errors and maintain consistency with the overload above (line 461), we should probably use `base::ClampRound()` here.
500));This is a slight oddity: the bucket will not actually be 500 in prod, but will land in the overflow bucket since `UmaHistogramPercentage` only supports values `[0, 100]`.
Could you add a comment here mentioning that we intentionally ignore the `.Scaled100` histogram for this scenario? For binned classifiers returning integers > 1, we rely on the `.Raw` metric, so the overflow here is expected behavior.
Related details
- Alexis Wu
- Salvador Guerrero Ramos
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexis Wu (Gerrit)
Alexis Wu added 2 comments![]( "Open in Gerrit")
base::UmaHistogramPercentage(histogram_name + ".Raw", result[i]);Since `result[i]` is a float (from the `ModelProvider::Response` vector), passing it directly to `UmaHistogramPercentage` (which takes an integer) relies on implicit C++ truncation. This means a value like 0.9 would be recorded as 0 rather than 1, and 79.9 would become 79.
To avoid off-by-one errors and maintain consistency with the overload above (line 461), we should probably use `base::ClampRound()` here.
Sounds good, implemented!
This is a slight oddity: the bucket will not actually be 500 in prod, but will land in the overflow bucket since `UmaHistogramPercentage` only supports values `[0, 100]`.
Could you add a comment here mentioning that we intentionally ignore the `.Scaled100` histogram for this scenario? For binned classifiers returning integers > 1, we rely on the `.Raw` metric, so the overflow here is expected behavior.
- Salvador Guerrero Ramos
- Tommy Nyquist
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexis Wu (Gerrit)
Alexis Wu added 1 comment![]( "Open in Gerrit")
PTAL, thank you. (sorry Tommy I do not know how to add a reviewer without overwriting your approval)
Related details
- Salvador Guerrero Ramos
- Shakti Sahu
- Tommy Nyquist
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Shakti Sahu (Gerrit)
Shakti Sahu voted Code-Review+1![]( "Open in Gerrit")
- Alexis Wu
- Salvador Guerrero Ramos
- Tommy Nyquist
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tommy Nyquist (Gerrit)
Tommy Nyquist voted and added 1 comment![]( "Open in Gerrit")
Votes added by Tommy Nyquist
| Code-Review | +1 |
1 comment
lgtm
Related details
- Alexis Wu
- Salvador Guerrero Ramos
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Salvador Guerrero Ramos (Gerrit)
Salvador Guerrero Ramos added 1 comment![]( "Open in Gerrit")
<owner>shakt...@chromium.org</owner>Could you add me and Sid to this owners list? Use my google.com account and Sid's chromium.org one.
Related details
- Alexis Wu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexis Wu (Gerrit)
Alexis Wu added 2 comments![]( "Open in Gerrit")
PTAL (the only change was added Sid and Salg to owners)
Could you add me and Sid to this owners list? Use my google.com account and Sid's chromium.org one.
- Salvador Guerrero Ramos
- Tommy Nyquist
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Salvador Guerrero Ramos (Gerrit)
Salvador Guerrero Ramos voted and added 2 comments![]( "Open in Gerrit")
Votes added by Salvador Guerrero Ramos
| Code-Review | +1 |
| Commit-Queue | +1 |
2 comments
LGTM % comment
name="SegmentationPlatform.ModelExecution.Result.{Index}.{SegmentationModel}.Scaled100"- Alexis Wu
- Tommy Nyquist
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tommy Nyquist (Gerrit)
Tommy Nyquist added 1 comment![]( "Open in Gerrit")
base::UmaHistogramPercentage(histogram_name + ".Raw", result[i]);Alexis WuSince `result[i]` is a float (from the `ModelProvider::Response` vector), passing it directly to `UmaHistogramPercentage` (which takes an integer) relies on implicit C++ truncation. This means a value like 0.9 would be recorded as 0 rather than 1, and 79.9 would become 79.
To avoid off-by-one errors and maintain consistency with the overload above (line 461), we should probably use `base::ClampRound()` here.
Sounds good, implemented!
Sorry, why did we remove the base::ClampRound() invocation again?
Related details
- Alexis Wu
Alexis Wu (Gerrit)
Alexis Wu added 1 comment![]( "Open in Gerrit")
base::UmaHistogramPercentage(histogram_name + ".Raw", result[i]);Alexis WuSince `result[i]` is a float (from the `ModelProvider::Response` vector), passing it directly to `UmaHistogramPercentage` (which takes an integer) relies on implicit C++ truncation. This means a value like 0.9 would be recorded as 0 rather than 1, and 79.9 would become 79.
To avoid off-by-one errors and maintain consistency with the overload above (line 461), we should probably use `base::ClampRound()` here.
Tommy NyquistSounds good, implemented!
Sorry, why did we remove the base::ClampRound() invocation again?
It's implemented in the latest patchset5.
Related details
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexis Wu (Gerrit)
Alexis Wu added 1 comment![]( "Open in Gerrit")
base::UmaHistogramPercentage(histogram_name + ".Raw", result[i]);Alexis WuSince `result[i]` is a float (from the `ModelProvider::Response` vector), passing it directly to `UmaHistogramPercentage` (which takes an integer) relies on implicit C++ truncation. This means a value like 0.9 would be recorded as 0 rather than 1, and 79.9 would become 79.
To avoid off-by-one errors and maintain consistency with the overload above (line 461), we should probably use `base::ClampRound()` here.
Tommy NyquistSounds good, implemented!
Alexis WuSorry, why did we remove the base::ClampRound() invocation again?
It's implemented in the latest patchset5.
nvm seems like I accidentally reversed it. Will do with the new add owner request :)
Related details
- Tommy Nyquist
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Salvador Guerrero Ramos (Gerrit)
Salvador Guerrero Ramos voted Code-Review+1![]( "Open in Gerrit")
- Alexis Wu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexis Wu (Gerrit)
Alexis Wu added 3 comments![]( "Open in Gerrit")
base::UmaHistogramPercentage(histogram_name + ".Raw", result[i]);Alexis WuSince `result[i]` is a float (from the `ModelProvider::Response` vector), passing it directly to `UmaHistogramPercentage` (which takes an integer) relies on implicit C++ truncation. This means a value like 0.9 would be recorded as 0 rather than 1, and 79.9 would become 79.
To avoid off-by-one errors and maintain consistency with the overload above (line 461), we should probably use `base::ClampRound()` here.
Tommy NyquistSounds good, implemented!
Alexis WuSorry, why did we remove the base::ClampRound() invocation again?
Alexis WuIt's implemented in the latest patchset5.
nvm seems like I accidentally reversed it. Will do with the new add owner request :)
Done
name="SegmentationPlatform.ModelExecution.Result.{Index}.{SegmentationModel}.Scaled100"Thanks! Could you do the same for this one?
- Salvador Guerrero Ramos
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexis Wu (Gerrit)
Alexis Wu added 1 comment![]( "Open in Gerrit")
The previous patchset test failed before because we expected 0.9 to return 0 in unittest previously without using the clampround. Now it has been implemented; I corrected the expected output to 1. Should be fixed now. Thank you!
Related details
- Salvador Guerrero Ramos
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Salvador Guerrero Ramos (Gerrit)
Salvador Guerrero Ramos voted Code-Review+1![]( "Open in Gerrit")
- Alexis Wu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tommy Nyquist (Gerrit)
Tommy Nyquist voted and added 1 comment![]( "Open in Gerrit")
Votes added by Tommy Nyquist
| Code-Review | +1 |
1 comment
lgtm
Related details
- Alexis Wu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexis Wu (Gerrit)
Alexis Wu voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Fix data loss in Segmentation model execution metrics
Existing metrics incorrectly assumed "binned classifiers" would only return integers. This caused [0, 1] float outputs (like those in FedCM) to be rounded to 0 or 1.
This CL introduces separate histograms for raw and scaled (*100) results to ensure proper granularity for all predictor types. The original histogram is deprecated with a TODO for removal.
- M components/segmentation_platform/internal/stats.cc
- M components/segmentation_platform/internal/stats_unittest.cc
- M tools/metrics/histograms/metadata/segmentation_platform/histograms.xml
Code-Review: +1 by Salvador Guerrero Ramos, +1 by Tommy Nyquist
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |