Fix off-by-one in aggregations #55920
Conversation
| @@ -102,7 +102,7 @@ def get_window_bounds( | |||
| closed: str | None = None, | |||
| step: int | None = None, | |||
| ) -> tuple[np.ndarray, np.ndarray]: | |||
| if center: | |||
| if center or self.window_size == 0: | |||
There was a problem hiding this comment.
Wouldn't this result in the same resulting offset as before? i.e.
In [1]: 0 - 1 // 2
Out[1]: 0There was a problem hiding this comment.
In your code 1 // 2 is getting evaluated first. The parentheses matter
>>> (0 - 1) // 2
-1There was a problem hiding this comment.
I verified this fixed the segfault via a local run of #55102 - hopefully can make progress soon to getting that up in CI since this is now 2 releases in a row where we have a segfault when cutting the release.
#55151 is a big step towards getting UBSAN/ASAN set up as the datetime code issues a ton of errors, and by design those tools stop at the first error encountered. If you have any time to help review that and push through would appreciate it
|
Thanks @WillAyd (for fixing my math mistake 😅 ) |
…55942) Backport PR #55920: Fix off-by-one in aggregations Co-authored-by: William Ayd <[email protected]>
ref #55713 (comment)
I'm not super familiar with this function so maybe there is a more intelligent way of doing this. But the segfault occurs when self.window_size == 0 because of these lines:
In the segfaulting example,
num_valuesis 20 for an array with 20 elements.endends up becoming 1-20 as does start with the window_size of 0. Later code then tries to index the 20th index in array, hence the address violation