Skip to content

Commit

Permalink
Merge pull request #1269 from rust-lang/summary-nan
Browse files Browse the repository at this point in the history 
Fix NaN handling in PR summary table
  • Loading branch information
@Kobzol
Kobzol authored Apr 3, 2022
2 parents 6d52897 + 7e8bb10 commit 501c5c4
Showing 1 changed file with 88 additions and 30 deletions.
118 changes: 88 additions & 30 deletions site/src/comparison.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -327,6 +327,10 @@ impl ComparisonSummary {
.count()
}

pub fn is_empty(&self) -> bool {
self.comparisons.is_empty()
}

fn arithmetic_mean<'a>(
&'a self,
changes: impl Iterator<Item = &'a TestResultComparison>,
Expand Down Expand Up @@ -473,7 +477,7 @@ pub fn write_summary_table(

writeln!(
result,
"| mean[^2] | {} | {} | {} | {} | {:.1}% |",
"| mean[^2] | {} | {} | {} | {} | {} |",
render_stat(primary.num_regressions, || Some(
primary.arithmetic_mean_of_regressions()
)),
Expand All @@ -486,21 +490,30 @@ pub fn write_summary_table(
render_stat(secondary.num_improvements, || Some(
secondary.arithmetic_mean_of_improvements()
)),
primary.arithmetic_mean_of_changes()
if primary.is_empty() {
"N/A".to_string()
} else {
format!("{:.1}%", primary.arithmetic_mean_of_changes())
}
)
.unwrap();

let largest_change = primary
.most_relevant_changes()
.iter()
.fold(0.0, |accum: f64, item| {
let change = item.map(|v| v.relative_change() * 100.0).unwrap_or(0.0);
accum.max(change)
});
let largest_change = if primary.is_empty() {
"N/A".to_string()
} else {
let change = primary
.most_relevant_changes()
.iter()
.fold(0.0, |accum: f64, item| {
let change = item.map(|v| v.relative_change() * 100.0).unwrap_or(0.0);
accum.max(change)
});
format!("{:.1}%", change)
};

writeln!(
result,
"| max | {} | {} | {} | {} | {:.1}% |",
"| max | {} | {} | {} | {} | {} |",
render_stat(primary.num_regressions, || primary
.largest_regression()
.map(|r| r.relative_change() * 100.0)),
Expand Down Expand Up @@ -1353,6 +1366,7 @@ fn compare_link(start: &ArtifactId, end: &ArtifactId) -> String {

#[cfg(test)]
mod tests {
use collector::category::Category;
use std::collections::HashSet;

use database::{ArtifactId, Profile, Scenario};
Expand All @@ -1363,12 +1377,12 @@ mod tests {
};

#[test]
fn summary_table_only_improvements() {
fn summary_table_only_improvements_primary() {
check_table(
vec![
("primary", 5.0, 10.0),
("primary", 5.0, 12.0),
("primary", 1.0, 3.0),
(Category::Primary, 5.0, 10.0),
(Category::Primary, 5.0, 12.0),
(Category::Primary, 1.0, 3.0),
],
r#"
| | Regressions 😿 <br />(primary) | Regressions 😿 <br />(secondary) | Improvements 🎉 <br />(primary) | Improvements 🎉 <br />(secondary) | All 😿 🎉 <br />(primary) |
Expand All @@ -1385,12 +1399,12 @@ mod tests {
}

#[test]
fn summary_table_only_regressions() {
fn summary_table_only_regressions_primary() {
check_table(
vec![
("primary", 5.0, 2.0),
("primary", 5.0, 1.0),
("primary", 4.0, 1.0),
(Category::Primary, 5.0, 2.0),
(Category::Primary, 5.0, 1.0),
(Category::Primary, 4.0, 1.0),
],
r#"
| | Regressions 😿 <br />(primary) | Regressions 😿 <br />(secondary) | Improvements 🎉 <br />(primary) | Improvements 🎉 <br />(secondary) | All 😿 🎉 <br />(primary) |
Expand All @@ -1406,14 +1420,58 @@ mod tests {
);
}

#[test]
fn summary_table_only_improvements_secondary() {
check_table(
vec![
(Category::Secondary, 5.0, 2.0),
(Category::Secondary, 5.0, 1.0),
(Category::Secondary, 4.0, 1.0),
],
r#"
| | Regressions 😿 <br />(primary) | Regressions 😿 <br />(secondary) | Improvements 🎉 <br />(primary) | Improvements 🎉 <br />(secondary) | All 😿 🎉 <br />(primary) |
|:---:|:---:|:---:|:---:|:---:|:---:|
| count[^1] | 0 | 0 | 0 | 3 | 0 |
| mean[^2] | N/A | N/A | N/A | -71.7% | N/A |
| max | N/A | N/A | N/A | -80.0% | N/A |
[^1]: *number of relevant changes*
[^2]: *the arithmetic mean of the percent change*
"#
.trim_start(),
);
}

#[test]
fn summary_table_only_regressions_secondary() {
check_table(
vec![
(Category::Secondary, 5.0, 10.0),
(Category::Secondary, 5.0, 12.0),
(Category::Secondary, 1.0, 3.0),
],
r#"
| | Regressions 😿 <br />(primary) | Regressions 😿 <br />(secondary) | Improvements 🎉 <br />(primary) | Improvements 🎉 <br />(secondary) | All 😿 🎉 <br />(primary) |
|:---:|:---:|:---:|:---:|:---:|:---:|
| count[^1] | 0 | 3 | 0 | 0 | 0 |
| mean[^2] | N/A | 146.7% | N/A | N/A | N/A |
| max | N/A | 200.0% | N/A | N/A | N/A |
[^1]: *number of relevant changes*
[^2]: *the arithmetic mean of the percent change*
"#
.trim_start(),
);
}

#[test]
fn summary_table_mixed_primary() {
check_table(
vec![
("primary", 10.0, 5.0),
("primary", 5.0, 10.0),
("primary", 1.0, 3.0),
("primary", 4.0, 1.0),
(Category::Primary, 10.0, 5.0),
(Category::Primary, 5.0, 10.0),
(Category::Primary, 1.0, 3.0),
(Category::Primary, 4.0, 1.0),
],
r#"
| | Regressions 😿 <br />(primary) | Regressions 😿 <br />(secondary) | Improvements 🎉 <br />(primary) | Improvements 🎉 <br />(secondary) | All 😿 🎉 <br />(primary) |
Expand All @@ -1433,12 +1491,12 @@ mod tests {
fn summary_table_mixed_primary_secondary() {
check_table(
vec![
("primary", 10.0, 5.0),
("primary", 5.0, 10.0),
("secondary", 5.0, 10.0),
("primary", 1.0, 3.0),
("secondary", 3.0, 1.0),
("primary", 4.0, 1.0),
(Category::Primary, 10.0, 5.0),
(Category::Primary, 5.0, 10.0),
(Category::Secondary, 5.0, 10.0),
(Category::Primary, 1.0, 3.0),
(Category::Secondary, 3.0, 1.0),
(Category::Primary, 4.0, 1.0),
],
r#"
| | Regressions 😿 <br />(primary) | Regressions 😿 <br />(secondary) | Improvements 🎉 <br />(primary) | Improvements 🎉 <br />(secondary) | All 😿 🎉 <br />(primary) |
Expand All @@ -1455,11 +1513,11 @@ mod tests {
}

// (category, before, after)
fn check_table(values: Vec<(&str, f64, f64)>, expected: &str) {
fn check_table(values: Vec<(Category, f64, f64)>, expected: &str) {
let mut primary_statistics = HashSet::new();
let mut secondary_statistics = HashSet::new();
for (index, (category, before, after)) in values.into_iter().enumerate() {
let target = if category == "primary" {
let target = if category == Category::Primary {
&mut primary_statistics
} else {
&mut secondary_statistics
Expand Down

0 comments on commit 501c5c4

Please sign in to comment.