[core][metric] improve histogram metrics midpoint calculation #57948
Conversation
Signed-off-by: justwph <[email protected]>
![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
This pull request correctly fixes a bug in the histogram midpoint calculation when bucket boundaries are negative. The change is simple and effective, and a new test case has been added to cover this scenario. My review includes a couple of suggestions to improve code structure and test organization for better long-term maintainability.
| lower_bound = 0.0 if buckets[0] > 0 else buckets[0] * 2.0 | ||
| self._histogram_bucket_midpoints[name].append( | ||
| lower_bound + buckets[0] / 2.0 | ||
| (lower_bound + buckets[0]) / 2.0 |
There was a problem hiding this comment.
While this change correctly fixes the midpoint calculation, the logic for calculating all bucket midpoints (lines 161-177) could be extracted into a separate private helper method, e.g., _calculate_histogram_bucket_midpoints(buckets). This would improve the readability and maintainability of register_histogram_metric by separating the registration logic from the midpoint calculation. It would also allow the midpoint calculation to be unit tested more directly.
| mock_meter.create_histogram.return_value = NoOpHistogram(name="neg_histogram") | ||
| recorder.register_histogram_metric( | ||
| name="neg_histogram", | ||
| description="Histogram with negative first boundary", | ||
| buckets=[-5.0, 0.0, 10.0], | ||
| ) | ||
|
|
||
| mids = recorder.get_histogram_bucket_midpoints("neg_histogram") | ||
| assert mids == pytest.approx([-7.5, -2.5, 5.0, 20.0]) |
There was a problem hiding this comment.
For better test isolation and clarity, it would be beneficial to move this test case for negative bucket boundaries into its own separate test function, for example test_register_histogram_metric_negative_boundary. This makes it easier to identify which specific scenario fails if the test breaks in the future.
ray-gardener
bot
added
core
|
@can-anyscale PTAL |
can-anyscale
left a comment
can-anyscale
left a comment
There was a problem hiding this comment.
Thanks for catching! This makes sense.
There’s a potential risk in the current midpoint calculation. It can be wrong when values are negative. Line 167: lower_bound + buckets[0] / 2.0 Line 171: (buckets[i] + buckets[i - 1]) / 2.0 I improved the formula and added a test to make sure it works. Signed-off-by: justwph <[email protected]> Signed-off-by: elliot-barn <[email protected]>
…oject#57948) There’s a potential risk in the current midpoint calculation. It can be wrong when values are negative. Line 167: lower_bound + buckets[0] / 2.0 Line 171: (buckets[i] + buckets[i - 1]) / 2.0 I improved the formula and added a test to make sure it works. Signed-off-by: justwph <[email protected]>
…oject#57948) There’s a potential risk in the current midpoint calculation. It can be wrong when values are negative. Line 167: lower_bound + buckets[0] / 2.0 Line 171: (buckets[i] + buckets[i - 1]) / 2.0 I improved the formula and added a test to make sure it works. Signed-off-by: justwph <[email protected]> Signed-off-by: Aydin Abiar <[email protected]>
3 participants
There’s a potential risk in the current midpoint calculation. It can be wrong when values are negative.
Line 167: lower_bound + buckets[0] / 2.0
Line 171: (buckets[i] + buckets[i - 1]) / 2.0
I improved the formula and added a test to make sure it works.