aoto-tech commented on PR #25289: URL: https://github.com/apache/datafusion/pull/25289#issuecomment-5665334722
> @aoto-tech thanks, you're right. The threshold only needs to catch sums that an underflowing square could have changed, and `1e-180` is far above that. `array_distance([1e-100], [0])` was taking the slow path for no reason. > > I've pushed a length-based threshold, as you suggested. A square that underflows is off by at most half the smallest subnormal value, so `len` of them move the sum by at most `len * 2^-1075`. Rescaling now happens only when the sum is below `len * 2^-1012`. Above that bound, the underflow error is less than `2^-63` of the sum, far below its rounding precision. > > I first tried the tightest version, `len * 2^-1021`. On vectors whose elements are around 1e-160, it left some rows 1 to 4 ulps away from the rescaled result. With the `2^10` margin I found no differences in about 436,000 random rows. Those rows had lengths 1 to 4096 and magnitudes from 1e-320 to 1e-90, including rows with subnormal squares. > > On 20,000 rows of 1536 elements around 1e-100, nothing is rescaled any more. The ratios to `main` went from 3.58x / 4.82x / 2.03x (distance / cosine / normalize) to 1.00x / 1.02x / 0.70x. Rows that really underflow, around 1e-200, still take the slow path. I've updated the PR description, and the change is a separate commit so it's easy to review. > > Was this close to the fix you had in mind? I'm happy to adjust. Nice, the commit looks good to me. Thanks for making the adjustment! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
