Re: Minor bug in src/port/rint.c

Hi everyone,

I believe that there is a small bug in src/port/rint.c when the input
parameter has a fractional part of 0.5 which is demonstrated by the
attached program. It appears that the PG version of rint() rounds in the
wrong direction with respect to glibc.

mca(at)mca-desktop:~$ ./test
rint(-1.5): -2.000000
pg_rint(-1.5): -1.000000
rint(1.5): 2.000000
pg_rint(1.5): 1.000000

The fix is, of course, to add an equals into the if() comparisons on
lines 21 and 26, so that when the fractional part is 0.5, it rounds in
the opposite direction instead.

I'm sure that this will have practically zero effect on the code,
however it may be worth applying for correctness and consistency with
other platform implementations.

ATB,

Mark.

--
ILande - Open Source Consultancy
http://www.ilande.co.uk

Mark Cave-Ayland <mark(dot)cave-ayland(at)ilande(dot)co(dot)uk> writes:
> I believe that there is a small bug in src/port/rint.c when the input
> parameter has a fractional part of 0.5 which is demonstrated by the
> attached program. It appears that the PG version of rint() rounds in the
> wrong direction with respect to glibc.

> The fix is, of course, to add an equals into the if() comparisons on
> lines 21 and 26, so that when the fractional part is 0.5, it rounds in
> the opposite direction instead.

Your proposed fix wouldn't make it act the same as glibc, only move the
differences around. I believe glibc's default behavior for the
ambiguous cases is "round to nearest even number". You propose
replacing "round towards zero", which is what our code currently does,
with "round away from zero", which really isn't likely to match any
platform's behavior. (The behaviors specified by IEEE are "to nearest
even", "towards zero", "towards minus infinity", and "towards plus
infinity", with the first being the typical default.)

Considering that probably every modern platform has rint(), I doubt
it's worth spending time on our stopgap version to try to make it
fully IEEE-compliant ...

regards, tom lane

Tom Lane wrote:
> Mark Cave-Ayland <mark(dot)cave-ayland(at)ilande(dot)co(dot)uk> writes:
>> I believe that there is a small bug in src/port/rint.c when the input
>> parameter has a fractional part of 0.5 which is demonstrated by the
>> attached program. It appears that the PG version of rint() rounds in the
>> wrong direction with respect to glibc.
>
>> The fix is, of course, to add an equals into the if() comparisons on
>> lines 21 and 26, so that when the fractional part is 0.5, it rounds in
>> the opposite direction instead.
>
> Your proposed fix wouldn't make it act the same as glibc, only move the
> differences around. I believe glibc's default behavior for the
> ambiguous cases is "round to nearest even number". You propose
> replacing "round towards zero", which is what our code currently does,
> with "round away from zero", which really isn't likely to match any
> platform's behavior. (The behaviors specified by IEEE are "to nearest
> even", "towards zero", "towards minus infinity", and "towards plus
> infinity", with the first being the typical default.)
>
> Considering that probably every modern platform has rint(), I doubt
> it's worth spending time on our stopgap version to try to make it
> fully IEEE-compliant ...

Except win32. (let's not get into the argument about modern platforms,
please, but it certainly is one of our most popular ones)

//Magnus

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> Tom Lane wrote:
>> Considering that probably every modern platform has rint(), I doubt
>> it's worth spending time on our stopgap version to try to make it
>> fully IEEE-compliant ...

> Except win32.

Hasn't it got something equivalent? This is IEEE-required behavior
I think.

regards, tom lane

Tom Lane wrote:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>> Tom Lane wrote:
>>> Considering that probably every modern platform has rint(), I doubt
>>> it's worth spending time on our stopgap version to try to make it
>>> fully IEEE-compliant ...
>
>> Except win32.
>
> Hasn't it got something equivalent? This is IEEE-required behavior
> I think.

Quite possibly - I haven't looked for it. I just added port/rint.c when
it was missing that one.

Something for the list of things to investigate, I guess...

//Magnus

On Sun, 2008-01-20 at 16:47 -0500, Tom Lane wrote:

> Your proposed fix wouldn't make it act the same as glibc, only move the
> differences around. I believe glibc's default behavior for the
> ambiguous cases is "round to nearest even number". You propose
> replacing "round towards zero", which is what our code currently does,
> with "round away from zero", which really isn't likely to match any
> platform's behavior. (The behaviors specified by IEEE are "to nearest
> even", "towards zero", "towards minus infinity", and "towards plus
> infinity", with the first being the typical default.)
>
> Considering that probably every modern platform has rint(), I doubt
> it's worth spending time on our stopgap version to try to make it
> fully IEEE-compliant ...

Hi Tom,

Right, I think I understand more about this now. My confusion stemmed
from reading the GNU documentation here:
http://www.gnu.org/software/libc/manual/html_node/Rounding-Functions.html where in rint() section it says "The default rounding mode is to round to the nearest integer". However, Wikipedia proved to be quite a fruitful resource, and agrees with what you stated which was "round to even number".

Interestingly, the article on rounding also points to this page
http://support.microsoft.com/kb/196652 which has some example code to
perform round to even number, or Banker's Rounding. The reason I was
looking into this is because PostGIS will require this for an MSVC Win32
build. I haven't yet tried the implementations given in the above page,
so I'm not sure whether they are any good or not...

The big question is, of course, how much difference does this make? Does
the current implementation exhibit different behaviour for certain date
calculations between Win32 and glibc platforms, and if so does it
matter? I guess at the end of the day, if it doesn't have any real
effect then there is no need to worry about it.

ATB,

Mark.

--
ILande - Open Source Consultancy
http://www.ilande.co.uk

Mark Cave-Ayland <mark(dot)cave-ayland(at)ilande(dot)co(dot)uk> writes:
> The big question is, of course, how much difference does this make?

Probably not a lot. If we can find an IEEE-compliant rounding function
on Windows, I'd be happy to see rint() fixed to call it; beyond that
I think it's not worth troubling with.

regards, tom lane

Added to TODO:

o Fix port/rint.c to be spec-compliant

http://archives.postgresql.org/pgsql-hackers/2008-01/msg00808.php

---------------------------------------------------------------------------

Mark Cave-Ayland wrote:
> Hi everyone,
>
> I believe that there is a small bug in src/port/rint.c when the input
> parameter has a fractional part of 0.5 which is demonstrated by the
> attached program. It appears that the PG version of rint() rounds in the
> wrong direction with respect to glibc.
>
> mca(at)mca-desktop:~$ ./test
> rint(-1.5): -2.000000
> pg_rint(-1.5): -1.000000
> rint(1.5): 2.000000
> pg_rint(1.5): 1.000000
>
> The fix is, of course, to add an equals into the if() comparisons on
> lines 21 and 26, so that when the fractional part is 0.5, it rounds in
> the opposite direction instead.
>
> I'm sure that this will have practically zero effect on the code,
> however it may be worth applying for correctness and consistency with
> other platform implementations.
>
>
> ATB,
>
> Mark.
>
> --
> ILande - Open Source Consultancy
> http://www.ilande.co.uk
>

[ Attachment, skipping... ]

>
> ---------------------------(end of broadcast)---------------------------
> TIP 2: Don't 'kill -9' the postmaster

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://postgres.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> Added to TODO:
> o Fix port/rint.c to be spec-compliant

Actually, the TODO I had in mind was entirely not that. Getting exact
spec compliance in a completely platform-independent fashion is probably
impossible, and is certainly not worth the trouble in view of the fact
that every SUS-compliant platform provides a working rint() already.
The proper wording of this item is

* Find a correct rint() substitute on Windows

regards, tom lane

Tom Lane wrote:

> The proper wording of this item is
>
> * Find a correct rint() substitute on Windows

Fixed.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.