Skip to content

Commit b863b1a

Browse files
nnethercotennethercote
authored
Merge pull request #1416 from nnethercote/range-in-comment
Show "range" instead of "max" in the GitHub comments.
2 parents 870445d + 9f2b2cf commit b863b1a

File tree

1 file changed

+121
-91
lines changed

1 file changed

+121
-91
lines changed

‎site/src/comparison.rs

Lines changed: 121 additions & 91 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@ use collector::Bound;
1313
use serde::{Deserialize, Serialize};
1414

1515
use database::CommitType;
16-
use std::cmp::Ordering;
1716
use std::collections::{HashMap, HashSet};
1817
use std::error::Error;
1918
use std::fmt::Write;
@@ -294,7 +293,7 @@ impl Metric {
294293
///
295294
/// This summary only includes changes that are significant and relevant (as determined by a change's magnitude).
296295
pub struct ArtifactComparisonSummary {
297-
/// Relevant comparisons ordered by magnitude from largest to smallest
296+
/// Relevant comparisons ordered from most negative to most positive
298297
relevant_comparisons: Vec<TestResultComparison>,
299298
/// The cached number of comparisons that are improvements
300299
num_improvements: usize,
@@ -321,9 +320,8 @@ impl ArtifactComparisonSummary {
321320
.collect::<Vec<_>>();
322321

323322
let cmp = |b1: &TestResultComparison, b2: &TestResultComparison| {
324-
b2.relative_change()
325-
.abs()
326-
.partial_cmp(&b1.relative_change().abs())
323+
b1.relative_change()
324+
.partial_cmp(&b2.relative_change())
327325
.unwrap_or(std::cmp::Ordering::Equal)
328326
};
329327
relevant_comparisons.sort_by(cmp);
@@ -439,16 +437,42 @@ impl ArtifactComparisonSummary {
439437
.filter(|c| c.is_regression())
440438
}
441439

440+
// This is the most negative result.
442441
fn largest_improvement(&self) -> Option<&TestResultComparison> {
443442
self.relevant_comparisons
444443
.iter()
445444
.find(|s| s.is_improvement())
446445
}
447446

447+
// This is the least negative result.
448+
fn smallest_improvement(&self) -> Option<&TestResultComparison> {
449+
self.relevant_comparisons
450+
.iter()
451+
.rfind(|s| s.is_improvement())
452+
}
453+
454+
// This is the most positive result.
448455
fn largest_regression(&self) -> Option<&TestResultComparison> {
456+
self.relevant_comparisons
457+
.iter()
458+
.rfind(|s| s.is_regression())
459+
}
460+
461+
// This is the least positive result.
462+
fn smallest_regression(&self) -> Option<&TestResultComparison> {
449463
self.relevant_comparisons.iter().find(|s| s.is_regression())
450464
}
451465

466+
// This may be an improvement or a regression.
467+
fn most_positive_change(&self) -> Option<&TestResultComparison> {
468+
self.relevant_comparisons.last()
469+
}
470+
471+
// This may be an improvement or a regression.
472+
fn most_negative_change(&self) -> Option<&TestResultComparison> {
473+
self.relevant_comparisons.first()
474+
}
475+
452476
/// The relevance level of the entire comparison
453477
pub fn is_relevant(&self) -> bool {
454478
!self.is_empty()
@@ -459,7 +483,19 @@ impl ArtifactComparisonSummary {
459483
}
460484

461485
pub fn largest_change(&self) -> Option<&TestResultComparison> {
462-
self.relevant_comparisons.first()
486+
if self.num_changes() == 0 {
487+
None
488+
} else {
489+
let most_pos = self.most_positive_change().unwrap();
490+
let most_neg = self.most_negative_change().unwrap();
491+
let most_pos_abs = most_pos.relative_change().abs();
492+
let most_neg_abs = most_neg.relative_change().abs();
493+
if most_neg_abs.partial_cmp(&most_pos_abs) == Some(std::cmp::Ordering::Greater) {
494+
Some(most_neg)
495+
} else {
496+
Some(most_pos)
497+
}
498+
}
463499
}
464500
}
465501

@@ -534,6 +570,15 @@ pub fn write_summary_table(
534570
.unwrap_or_else(|| "-".to_string())
535571
}
536572

573+
fn render_range<F: FnOnce() -> (f64, f64)>(count: usize, calculate: F) -> String {
574+
if count > 0 {
575+
let (a, b) = calculate();
576+
format!("[{a:.1}%, {b:.1}%]")
577+
} else {
578+
"-".to_string()
579+
}
580+
}
581+
537582
// (label, mean, max, count)
538583
let mut column_data = vec![];
539584

@@ -567,54 +612,39 @@ pub fn write_summary_table(
567612
},
568613
]);
569614

570-
let largest_change = if primary.is_empty() {
571-
"-".to_string()
572-
} else {
573-
let largest_improvement = primary
574-
.largest_improvement()
575-
.map(|c| c.relative_change())
576-
.unwrap_or(0.0);
577-
let largest_regression = primary
578-
.largest_regression()
579-
.map(|c| c.relative_change())
580-
.unwrap_or(0.0);
581-
let change = if largest_improvement
582-
.abs()
583-
.partial_cmp(&largest_regression.abs())
584-
.unwrap_or(Ordering::Equal)
585-
!= Ordering::Less
586-
{
587-
largest_improvement
588-
} else {
589-
largest_regression
590-
};
591-
592-
format!("{:.1}%", change * 100.0)
593-
};
594-
595-
// max
615+
// range
616+
let rel_change = |r: Option<&TestResultComparison>| r.unwrap().relative_change() * 100.0;
596617
column_data.push(vec![
597-
render_stat(primary.num_regressions, || {
598-
primary
599-
.largest_regression()
600-
.map(|r| r.relative_change() * 100.0)
618+
render_range(primary.num_regressions, || {
619+
(
620+
rel_change(primary.smallest_regression()),
621+
rel_change(primary.largest_regression()),
622+
)
601623
}),
602-
render_stat(secondary.num_regressions, || {
603-
secondary
604-
.largest_regression()
605-
.map(|r| r.relative_change() * 100.0)
624+
render_range(secondary.num_regressions, || {
625+
(
626+
rel_change(secondary.smallest_regression()),
627+
rel_change(secondary.largest_regression()),
628+
)
606629
}),
607-
render_stat(primary.num_improvements, || {
608-
primary
609-
.largest_improvement()
610-
.map(|r| r.relative_change() * 100.0)
630+
render_range(primary.num_improvements, || {
631+
(
632+
rel_change(primary.largest_improvement()),
633+
rel_change(primary.smallest_improvement()),
634+
)
611635
}),
612-
render_stat(secondary.num_improvements, || {
613-
secondary
614-
.largest_improvement()
615-
.map(|r| r.relative_change() * 100.0)
636+
render_range(secondary.num_improvements, || {
637+
(
638+
rel_change(secondary.largest_improvement()),
639+
rel_change(secondary.smallest_improvement()),
640+
)
641+
}),
642+
render_range(primary.num_regressions + primary.num_improvements, || {
643+
(
644+
rel_change(primary.most_negative_change()),
645+
rel_change(primary.most_positive_change()),
646+
)
616647
}),
617-
largest_change,
618648
]);
619649

620650
// count
@@ -631,7 +661,7 @@ pub fn write_summary_table(
631661
let column_labels = [
632662
metric,
633663
format!("mean{}", if with_footnotes { "[^1]" } else { "" }),
634-
"max".to_string(),
664+
"range".to_string(),
635665
format!("count{}", if with_footnotes { "[^2]" } else { "" }),
636666
];
637667
let counts: Vec<usize> = column_labels.iter().map(|s| s.chars().count()).collect();
@@ -1385,11 +1415,11 @@ mod tests {
13851415
(Category::Primary, 1.0, 3.0),
13861416
],
13871417
r#"
1388-
| Regressions ❌ <br /> (primary) | 146.7% | 200.0% | 3 |
1389-
| Regressions ❌ <br /> (secondary) | - | - | 0 |
1390-
| Improvements ✅ <br /> (primary) | - | - | 0 |
1391-
| Improvements ✅ <br /> (secondary) | - | - | 0 |
1392-
| All ❌✅ (primary) | 146.7% | 200.0% | 3 |
1418+
| Regressions ❌ <br /> (primary) | 146.7% | [100.0%, 200.0%] | 3 |
1419+
| Regressions ❌ <br /> (secondary) | - | - | 0 |
1420+
| Improvements ✅ <br /> (primary) | - | - | 0 |
1421+
| Improvements ✅ <br /> (secondary) | - | - | 0 |
1422+
| All ❌✅ (primary) | 146.7% | [100.0%, 200.0%] | 3 |
13931423
"#
13941424
.trim_start(),
13951425
);
@@ -1404,11 +1434,11 @@ mod tests {
14041434
(Category::Primary, 4.0, 1.0),
14051435
],
14061436
r#"
1407-
| Regressions ❌ <br /> (primary) | - | - | 0 |
1408-
| Regressions ❌ <br /> (secondary) | - | - | 0 |
1409-
| Improvements ✅ <br /> (primary) | -71.7% | -80.0% | 3 |
1410-
| Improvements ✅ <br /> (secondary) | - | - | 0 |
1411-
| All ❌✅ (primary) | -71.7% | -80.0% | 3 |
1437+
| Regressions ❌ <br /> (primary) | - | - | 0 |
1438+
| Regressions ❌ <br /> (secondary) | - | - | 0 |
1439+
| Improvements ✅ <br /> (primary) | -71.7% | [-80.0%, -60.0%] | 3 |
1440+
| Improvements ✅ <br /> (secondary) | - | - | 0 |
1441+
| All ❌✅ (primary) | -71.7% | [-80.0%, -60.0%] | 3 |
14121442
"#
14131443
.trim_start(),
14141444
);
@@ -1423,11 +1453,11 @@ mod tests {
14231453
(Category::Secondary, 4.0, 1.0),
14241454
],
14251455
r#"
1426-
| Regressions ❌ <br /> (primary) | - | - | 0 |
1427-
| Regressions ❌ <br /> (secondary) | - | - | 0 |
1428-
| Improvements ✅ <br /> (primary) | - | - | 0 |
1429-
| Improvements ✅ <br /> (secondary) | -71.7% | -80.0% | 3 |
1430-
| All ❌✅ (primary) | - | - | 0 |
1456+
| Regressions ❌ <br /> (primary) | - | - | 0 |
1457+
| Regressions ❌ <br /> (secondary) | - | - | 0 |
1458+
| Improvements ✅ <br /> (primary) | - | - | 0 |
1459+
| Improvements ✅ <br /> (secondary) | -71.7% | [-80.0%, -60.0%] | 3 |
1460+
| All ❌✅ (primary) | - | - | 0 |
14311461
"#
14321462
.trim_start(),
14331463
);
@@ -1442,11 +1472,11 @@ mod tests {
14421472
(Category::Secondary, 1.0, 3.0),
14431473
],
14441474
r#"
1445-
| Regressions ❌ <br /> (primary) | - | - | 0 |
1446-
| Regressions ❌ <br /> (secondary) | 146.7% | 200.0% | 3 |
1447-
| Improvements ✅ <br /> (primary) | - | - | 0 |
1448-
| Improvements ✅ <br /> (secondary) | - | - | 0 |
1449-
| All ❌✅ (primary) | - | - | 0 |
1475+
| Regressions ❌ <br /> (primary) | - | - | 0 |
1476+
| Regressions ❌ <br /> (secondary) | 146.7% | [100.0%, 200.0%] | 3 |
1477+
| Improvements ✅ <br /> (primary) | - | - | 0 |
1478+
| Improvements ✅ <br /> (secondary) | - | - | 0 |
1479+
| All ❌✅ (primary) | - | - | 0 |
14501480
"#
14511481
.trim_start(),
14521482
);
@@ -1462,11 +1492,11 @@ mod tests {
14621492
(Category::Primary, 4.0, 1.0),
14631493
],
14641494
r#"
1465-
| Regressions ❌ <br /> (primary) | 150.0% | 200.0% | 2 |
1466-
| Regressions ❌ <br /> (secondary) | - | - | 0 |
1467-
| Improvements ✅ <br /> (primary) | -62.5% | -75.0% | 2 |
1468-
| Improvements ✅ <br /> (secondary) | - | - | 0 |
1469-
| All ❌✅ (primary) | 43.8% | 200.0% | 4 |
1495+
| Regressions ❌ <br /> (primary) | 150.0% | [100.0%, 200.0%] | 2 |
1496+
| Regressions ❌ <br /> (secondary) | - | - | 0 |
1497+
| Improvements ✅ <br /> (primary) | -62.5% | [-75.0%, -50.0%] | 2 |
1498+
| Improvements ✅ <br /> (secondary) | - | - | 0 |
1499+
| All ❌✅ (primary) | 43.8% | [-75.0%, 200.0%] | 4 |
14701500
"#
14711501
.trim_start(),
14721502
);
@@ -1484,11 +1514,11 @@ mod tests {
14841514
(Category::Primary, 4.0, 1.0),
14851515
],
14861516
r#"
1487-
| Regressions ❌ <br /> (primary) | 150.0% | 200.0% | 2 |
1488-
| Regressions ❌ <br /> (secondary) | 100.0% | 100.0% | 1 |
1489-
| Improvements ✅ <br /> (primary) | -62.5% | -75.0% | 2 |
1490-
| Improvements ✅ <br /> (secondary) | -66.7% | -66.7% | 1 |
1491-
| All ❌✅ (primary) | 43.8% | 200.0% | 4 |
1517+
| Regressions ❌ <br /> (primary) | 150.0% | [100.0%, 200.0%] | 2 |
1518+
| Regressions ❌ <br /> (secondary) | 100.0% | [100.0%, 100.0%] | 1 |
1519+
| Improvements ✅ <br /> (primary) | -62.5% | [-75.0%, -50.0%] | 2 |
1520+
| Improvements ✅ <br /> (secondary) | -66.7% | [-66.7%, -66.7%] | 1 |
1521+
| All ❌✅ (primary) | 43.8% | [-75.0%, 200.0%] | 4 |
14921522
"#
14931523
.trim_start(),
14941524
);
@@ -1502,11 +1532,11 @@ mod tests {
15021532
(Category::Primary, 5.0, 6.0),
15031533
],
15041534
r#"
1505-
| Regressions ❌ <br /> (primary) | 20.0% | 20.0% | 1 |
1506-
| Regressions ❌ <br /> (secondary) | - | - | 0 |
1507-
| Improvements ✅ <br /> (primary) | -50.0% | -50.0% | 1 |
1508-
| Improvements ✅ <br /> (secondary) | - | - | 0 |
1509-
| All ❌✅ (primary) | -15.0% | -50.0% | 2 |
1535+
| Regressions ❌ <br /> (primary) | 20.0% | [20.0%, 20.0%] | 1 |
1536+
| Regressions ❌ <br /> (secondary) | - | - | 0 |
1537+
| Improvements ✅ <br /> (primary) | -50.0% | [-50.0%, -50.0%] | 1 |
1538+
| Improvements ✅ <br /> (secondary) | - | - | 0 |
1539+
| All ❌✅ (primary) | -15.0% | [-50.0%, 20.0%] | 2 |
15101540
"#
15111541
.trim_start(),
15121542
);
@@ -1520,11 +1550,11 @@ mod tests {
15201550
(Category::Primary, 6.0, 5.0),
15211551
],
15221552
r#"
1523-
| Regressions ❌ <br /> (primary) | 100.0% | 100.0% | 1 |
1524-
| Regressions ❌ <br /> (secondary) | - | - | 0 |
1525-
| Improvements ✅ <br /> (primary) | -16.7% | -16.7% | 1 |
1526-
| Improvements ✅ <br /> (secondary) | - | - | 0 |
1527-
| All ❌✅ (primary) | 41.7% | 100.0% | 2 |
1553+
| Regressions ❌ <br /> (primary) | 100.0% | [100.0%, 100.0%] | 1 |
1554+
| Regressions ❌ <br /> (secondary) | - | - | 0 |
1555+
| Improvements ✅ <br /> (primary) | -16.7% | [-16.7%, -16.7%] | 1 |
1556+
| Improvements ✅ <br /> (secondary) | - | - | 0 |
1557+
| All ❌✅ (primary) | 41.7% | [-16.7%, 100.0%] | 2 |
15281558
"#
15291559
.trim_start(),
15301560
);
@@ -1574,7 +1604,7 @@ mod tests {
15741604

15751605
let mut result = String::new();
15761606
write_summary_table(&primary, &secondary, true, true, &mut result);
1577-
let header = "| (instructions:u) | mean[^1] | max | count[^2] |\n|:----------------:|:--------:|:---:|:---------:|\n";
1607+
let header = "| (instructions:u) | mean[^1] | range | count[^2] |\n|:----------------:|:--------:|:-----:|:---------:|\n";
15781608
assert_eq!(result, format!("{header}{expected}"));
15791609
}
15801610
}

0 commit comments

Comments
 (0)