gui: change HistogramView bucket sizes#9564
gui: change HistogramView bucket sizes#9564maliberty merged 4 commits intoThe-OpenROAD-Project:masterfrom
Conversation
Signed-off-by: LucasYuki <lucasyuki@yahoo.com.br>
Signed-off-by: LucasYuki <lucasyuki@yahoo.com.br>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request refactors the histogram bucket size calculation to use a standard algorithm for generating "nice" intervals, which improves code clarity and the visual quality of the histogram. However, I've identified two critical edge-case scenarios that could lead to application crashes. My review includes suggestions to address these issues by handling cases where the data range is zero, which would otherwise cause mathematical errors, and by ensuring the histogram is always configured with at least one bin to prevent errors in downstream processing.
| const float bin_min = std::floor(min_slack / bin_interval) * bin_interval; | ||
| const float bin_max = std::ceil(max_slack / bin_interval) * bin_interval; | ||
|
|
||
| const int bins = (bin_max - bin_min) / bin_interval; |
There was a problem hiding this comment.
If min_slack and max_slack are equal, bin_min can equal bin_max. This results in bins being 0 due to integer truncation. Passing 0 for the number of bins to utl::Histogram will cause a crash later when getMaxBinCount() is called on an empty bins vector. You should ensure there's at least one bin. Using std::round is also safer for floating-point-to-integer conversion.
| const int bins = (bin_max - bin_min) / bin_interval; | |
| const int bins = std::max(1, static_cast<int>(std::round((bin_max - bin_min) / bin_interval))); |
| { | ||
| if (exact_interval < 10) { | ||
| return std::ceil(exact_interval); | ||
| } | ||
|
|
||
| float snap_interval = 0; | ||
| const int digits = computeNumberOfDigits(exact_interval); | ||
|
|
||
| while (snap_interval < exact_interval) { | ||
| snap_interval += 5 * std::pow(10, digits - 2); | ||
| int exp = std::floor(std::log10(exact_interval)); | ||
| precision_count_ = std::max(-exp, 0); | ||
|
|
||
| double mag = std::pow(10.0, exp); | ||
| double residual = exact_interval / mag; | ||
|
|
||
| double nice_coeff; | ||
| if (residual < 1.5) { | ||
| nice_coeff = 1.0; | ||
| } else if (residual < 3.0) { | ||
| nice_coeff = 2.0; | ||
| } else if (residual < 7.0) { | ||
| nice_coeff = 5.0; | ||
| } else { | ||
| nice_coeff = 10.0; | ||
| } | ||
|
|
||
| return snap_interval; | ||
| return nice_coeff * mag; | ||
| } |
There was a problem hiding this comment.
If exact_interval is zero or negative (which can happen if max_slack equals min_slack), calling std::log10 will result in an error (-inf or NaN). This propagates through the calculations and can lead to a crash. Please add a guard to handle this edge case.
{
if (exact_interval <= 0.0f) {
return 1.0f;
}
int exp = std::floor(std::log10(exact_interval));
precision_count_ = std::max(-exp, 0);
double mag = std::pow(10.0, exp);
double residual = exact_interval / mag;
double nice_coeff;
if (residual < 1.5) {
nice_coeff = 1.0;
} else if (residual < 3.0) {
nice_coeff = 2.0;
} else if (residual < 7.0) {
nice_coeff = 5.0;
} else {
nice_coeff = 10.0;
}
return nice_coeff * mag;
}
|
clang-tidy review says "All clean, LGTM! 👍" |
Signed-off-by: LucasYuki <lucasyuki@yahoo.com.br>
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
3 participants
Closes #9387
Implements suggested changes in the charts histogram.