gui: fix bad behavior in Charts for large number of bins

[AcKoucher]

Fix #4447

Changes here:

1. Changing the mechanism we use to populate the bins: using a default number of bins to compute bin range.
2. Changing category axis for value axis - x axis - in order to have labels on the ticks;
3. Snapping x and y values to round numbers;
4. Adding a tool-tip when hovering for displaying number of endpoints in a certain interval and the interval's boundaries.

[github-actions[bot]]

clang-tidy review says "All clean, LGTM! :+1:"

[maliberty]

Please include a screen shot of what the widget now looks like for a large number of bins.

[maliberty]

If you can include the tool tip in the image that would be nice

[AcKoucher]

[github-actions[bot]]

clang-tidy review says "All clean, LGTM! :+1:"

[maliberty]

could you just have

Number of endpoints: 14
Interval [130, 140) ps

[maliberty]

For the x-axis can we use axisX.setLabelsAngle(-90) (from https://stackoverflow.com/questions/51784292/display-x-labels-vertically)

[github-actions[bot]]

clang-tidy review says "All clean, LGTM! :+1:"

[maliberty]

I find the digit compensator scheme somewhat hard to follow. Would you say more about what you are trying to accomplish.

[AcKoucher]

The idea would be to limit the number of bars based of the number of digits of the amount of buckets we'd get from the conversion we were doing before (casting the actual slack float value - in the time unit we get from sta - to int)

So maxdigits = 1 --> 9 buckets maxdigits = 2 --> 99 buckets ..

A dummy example of what I'm trying to accomplish would be:

for a max positive slack of 1243,5ns and supposing this is the max abs slack:

Previously, that would imply positive labels like: [0 1), [1 2), ... [1243 1244) ns Apparently QtCharts can't handle this large number of bars and it shows that weird behavior we see in #4447

With these changes we limit the number of buckets to be made, at max, of 2 digits, so we would have: [0 1), [1 2), ... [12, 13) ns * 10^2.

As the number of digits in the max abs slack is bigger than 2 (4 > 2), the compensator 10^2 is computed as 10^(max_abs_slack_number_of_digits - maxdigits).

[github-actions[bot]]

clang-tidy review says "All clean, LGTM! :+1:"

[github-actions[bot]]

clang-tidy review says "All clean, LGTM! :+1:"

[github-actions[bot]]

clang-tidy review says "All clean, LGTM! :+1:"

[github-actions[bot]]

clang-tidy review says "All clean, LGTM! :+1:"

[AcKoucher]

asap7/aes_lvt result after changes:

This is ready for review

[rovinski]

This is a nit but is there some way to get the axes to round to nice numbers? Like a y-axis that stops at 120 in increments of 10 instead of 8, and a bin range of [0,15) instead of [0,15.3)?

[AcKoucher]

@rovinski As for the bin range, I avoided snapping to round numbers like [0 15), because for ns that may be too coarse. However I agree that having one decimal place makes it a bit odd still...

As for the y_axis, yes that's possible. As we didn't have the tooltip before - and therefore we couldn't see the specif amount of endpoints in each bin - I chose to use the bin with more endpoints to be the max and just set a default tick count for the axis. Perhaps now that the tooltip is there, we could go with an approach of having just round numbers in y axis and, if the user wants to know the specif amount in a certain range, the tooltip will help.

[maliberty]

I think we should try to snap the X-axis bins. You could always use ps if that makes the snapping easier.

[oharboe]

asap7/aes_lvt result after changes:

This is ready for review

Nits: The labels on the horizontal axis are very busy and they repeat information(end of one is start of next)

Look at the histogram on wikipedia... It isn't important to communicate start/end of a bin, just plot integer ps(or ns) on the horizontal axis... Same for vertical axis, use nice round numbers...

https://en.wikipedia.org/wiki/Histogram

[github-actions[bot]]

clang-tidy review says "All clean, LGTM! :+1:"

[oharboe]

@AcKoucher I have positive slack and the chart shows an empty area. Can the xaxis min be made to be the largest slack?

I've also seen a histogram flipped: the worst slack is on the left. This feels a bit more natural as an English speaker: read left to right.

[AcKoucher]

@oharboe With the changes in this PR we'll be having a fixed number of bins and the interval will be computed from it. The idea is that subsequently we'll be able to add an interactive option for the user to chose a bin width/number.

It's hard to evaluate what's going on for cases like the one in your image, because in the original code, as there was no limit for the amount of bins (and each integer unit would correspond to a tick in the x axis), QtCharts couldn't handle an axis with so many values, leading to not only this odd legend being displayed in the origin of the widget, but also compromising the bars.

[maliberty]

worst (most negative) slack should be on the left. That's how it was before, has that changed?

[AcKoucher]

That hasn't changed.

https://github.com/The-OpenROAD-Project/OpenROAD/blob/a38447fde75615a7f2d801b6d848badb01ddf26a/src/gui/src/chartsWidget.cpp#L277

We populate the queue that holds the negative slack count with push_front, so we can push them in ascending order when actually filling the neg BarSet: https://github.com/The-OpenROAD-Project/OpenROAD/blob/a38447fde75615a7f2d801b6d848badb01ddf26a/src/gui/src/chartsWidget.cpp#L175-L177

Most negative slack will be on the left.

[oharboe]

worst (most negative) slack should be on the left. That's how it was before, has that changed?

Yes, but are the empty positive slack bins on the left pruned?

Otherwise you get a lot of empty bins on the left when there is positive slack...

[AcKoucher]

Yes, there will only be empty bins if there is subsequently a non-empty bin. That's why these checks and the comment when populating the queues:

https://github.com/The-OpenROAD-Project/OpenROAD/blob/a38447fde75615a7f2d801b6d848badb01ddf26a/src/gui/src/chartsWidget.cpp#L274-L290

Valid position of the queue = "There are still bins with non-zero values ahead"

[oharboe]

Yes, there will only be empty bins if there is subsequently a non-empty bin. That's why these checks and the comment when populating the queues:

I see. So lots of "empty" space visible on the left in my case means that there were a few slowest path outliers then before the bulk of the timing paths?

[AcKoucher]

I think it's hard to tell, because I see no way of knowing whether of not we're actually seeing all the bins in your image. My suspicions are that the widget was not showing the data correctly, because the framework itself has some limitations regarding the number of categories/values on the x axis.

Running with the new version should give us some insight.

[oharboe]

@maliberty @AcKoucher I don't think we have to wip this PR to death... If this is a step forward, it's OK to do another round when it has received broader usability testing?

[oharboe]

@maliberty @AcKoucher it would be great if the histogram was useful at the floorplan level. Fast turnaround times... Ignore the pathological slack and maybe it is telling me something that I can iterate quickly on?

Elipsis instead of numbers on horizontal axis.

This slack histogram is at the floorplan level, so the slack values are pathological...

[oharboe]

The tooltip disappears after ca. 1000ms when the mouse pointer is not being moved. If you focus on another window, then hover over the bin, then the tooltip doesn't go away when the mouse isn't being moved.

[oharboe]

I think it would look better if there were nice round numbers chosen for the labels on the horizontal axis independent of the bins.

Still elipsis with more reasonable values for slack.

[AcKoucher]

As for the elipsis, if you make the widget bigger stretching it horizontally the labels should appear, however perhaps we could have a smaller default number of buckets so they appear always.

[oharboe]

As for the elipsis, if you make the widget bigger stretching it horizontally the labels should appear, however perhaps we could have a smaller default number of buckets so they appear always.

I think the solution on wikipedia is better. Just decouple the bins from the labels on the horizontal axis and choose nice round numbers for labels.

I can get the numbers to show, but I think it would be better if it just worked out of the box...

I think there is little value in showing end/beginning values for each bucket. I think it would be useful to be able to click on the bucket and go to the Timing Report view and show the paths in that range.

[AcKoucher]

I think changing the default number of bins and improving the values on x axis is ok for adding to this PR.

I'll wait for @maliberty regarding the x axis labels displaying. Qt apparently doesn't provide a good alternative for QtCharts bars to be used with numeric values in the horizontal axis, so I couldn't find a good solution for decoupling the bins from the labels. I do agree with you it should be that way, but perhaps this will require a more elaborated solution.

[github-actions[bot]]

clang-tidy review says "All clean, LGTM! :+1:"

[AcKoucher]

I found a way to decouple the labels. Also snaping x value to round numbers and using less bins: