public inbox for [email protected]
help / color / mirror / Atom feedRe: [PATCH] Fix overflow and underflow in regr_r2()
2+ messages / 1 participants
[nested] [flat]
* Re: [PATCH] Fix overflow and underflow in regr_r2()
@ 2026-05-23 01:14 Chengpeng Yan <[email protected]>
1 sibling, 0 replies; 2+ messages in thread
From: Chengpeng Yan @ 2026-05-23 01:14 UTC (permalink / raw)
To: Dean Rasheed <[email protected]>; +Cc: pgsql-hackers; Tom Lane <[email protected]>
Hi,
> On May 16, 2026, at 17:39, Dean Rasheed <[email protected]> wrote:
>
>> corr() already has a stabilized calculation for the same Sxx * Syy
>> denominator scale. This patch factors that into a helper and lets
>> regr_r2() use it as a fallback when one of its direct products has
>> rounded to zero or infinity. Otherwise, regr_r2() keeps the existing
>> direct formula.
>
> The comments need work -- in particular float8_regr_r2() needs a
> comment explaining the new overflow/underflow checks, similar to the
> comment in float8_corr(). In fact, doing that, I think it's preferable
> to just keep this change local to float8_regr_r2(), rather than
> refactoring into a helper function for just a few lines of code.
>
> This new check in float8_regr_r2():
>
> + if (Sxy == 0 && !isnan(Sxx) && !isnan(Syy))
> + PG_RETURN_FLOAT8(0.0);
>
> seems pointless. It's optimising for a special case that will very
> rarely occur in practice, and which is handled fine by the general
> code. We don't want to slow down the common code path for such rare
> special cases.
>
> I noticed that this new overflow test case:
>
> +SELECT regr_r2(1e154::float8 * g, 1e154::float8 * g)
> + FROM generate_series(1, 2) g;
> + regr_r2
> +---------
> + 1
> +(1 row)
>
> only produces 1 because it's run with a reduced extra_float_digits
> value. I think it's better to use the test values "1e100 + g * 1e95",
> which still trigger the overflow on HEAD, but more reliably produce 1,
> regardless of the extra_float_digits setting, making it less likely
> that there will be variations between platforms. That's also more
> consistent with the other nearby test cases.
Thanks for the thorough review and feedback — I learned a lot from it!
> Attached is a v2 patch with those changes, plus a little more tidying
> up of the regression tests.
v2 LGTM. Thanks for the updates and test cleanup.
--
Best regards,
Chengpeng Yan
^ permalink raw reply [nested|flat] 2+ messages in thread
* Re: [PATCH] Fix overflow and underflow in regr_r2()
@ 2026-05-23 02:42 Chengpeng Yan <[email protected]>
1 sibling, 0 replies; 2+ messages in thread
From: Chengpeng Yan @ 2026-05-23 02:42 UTC (permalink / raw)
To: Dean Rasheed <[email protected]>; +Cc: Tom Lane <[email protected]>; pgsql-hackers
Hi,
> On May 17, 2026, at 17:16, Dean Rasheed <[email protected]> wrote:
>
> OK, here's a more complete patch along those lines, intended to apply
> on top of the regr_r2() patch.
Thanks for the regr_intercept.patch. The approach looks good to me.
I only noticed a few small things:
1. The patch file seems to have a format issue and doesn't apply
directly. `git apply` reports:
```
error: git apply: bad git-diff - expected /dev/null on line 2
```
2. `dy` seems a bit hard to understand. Perhaps `offset`, as used in the
earlier sketch, would be clearer?
3. Do we need to add tests for the underflow path, and perhaps for the
Inf/NaN guard?
Other than that, this looks good to me.
--
Best regards,
Chengpeng Yan
^ permalink raw reply [nested|flat] 2+ messages in thread
end of thread, other threads:[~2026-05-23 02:42 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-05-23 01:14 ` Chengpeng Yan <[email protected]>
2026-05-23 02:42 ` Chengpeng Yan <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox