-
Notifications
You must be signed in to change notification settings - Fork 125
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
A better explanation for change in differentials #294
base: main
Are you sure you want to change the base?
Changes from all commits
8973a5c
4c6b42b
3f7ed42
9118bd4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -616,21 +616,30 @@ where | |
"{} ({} {}, {:.2}%)", | ||
function, samples_txt, opt.count_name, pct | ||
), | ||
// Special case delta == 0 so we don't format percentage with a + sign. | ||
Some(delta) if delta == 0 => write!( | ||
buffer, | ||
"{} ({} {}, {:.2}%; 0.00%)", | ||
function, samples_txt, opt.count_name, pct, | ||
), | ||
Some(mut delta) => { | ||
if opt.negate_differentials { | ||
delta = -delta; | ||
} | ||
let delta_pct = (100 * delta) as f64 / (timemax as f64 * opt.factor); | ||
Some(delta) => { | ||
let samples = frame.end_time as isize - frame.start_time as isize; | ||
let (old, new) = if opt.negate_differentials { | ||
(samples, samples - delta) | ||
} else { | ||
(samples - delta, samples) | ||
}; | ||
let change = match (old, new) { | ||
(0, _) => "all newly added".to_string(), | ||
(_, 0) => "were all removed".to_string(), | ||
(x, y) if x == y => "".to_string(), | ||
(old, new) => { | ||
let (ratio, compared_to) = if opt.negate_differentials { | ||
(old as f64 / new as f64, "new") | ||
} else { | ||
(new as f64 / old as f64, "old") | ||
}; | ||
format!(" = {ratio:.3} × {compared_to} {}", opt.count_name) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure I follow the phrasing here. If, for example, we spend 2x as many samples in some frame, this will look like (I think):
Which I think is maybe overly concise. It doesn't include the function name (is that printed elsewhere?), and the old or new sample counts. I think we'll want to include at least the function name and one of the two absolute numbers. I'm also not sure if "old" and "new" are understandable labels in this instance (esp. for negate_differentials) — they may even be unnecessary. I think "2×cycles" may actually be sufficient. |
||
} | ||
}; | ||
write!( | ||
buffer, | ||
"{} ({} {}, {:.2}%; {:+.2}%)", | ||
function, samples_txt, opt.count_name, pct, delta_pct | ||
"{} ({} {} {}, {:.2}%)", | ||
function, samples_txt, opt.count_name, change, pct | ||
) | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if these labels still make sense if
negate_differentials
is on.