-
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?
A better explanation for change in differentials #294
Conversation
… calculate diff there for some reason.
Any thoughts on this approach? |
(0, _) => "all newly added".to_string(), | ||
(_, 0) => "were all removed".to_string(), |
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.
} 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 comment
The 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):
= 2 × old cycles
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.
…al-flamegraph-explanations
An example of how to fix #293
To complete: