Thread: 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@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
Mark Cave-Ayland <mark.cave-ayland@ilande.co.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.cave-ayland@ilande.co.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@hagander.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@hagander.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 roundingmode is to round to the nearest integer". However, Wikipedia proved to be quite a fruitful resource, and agrees withwhat 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.cave-ayland@ilande.co.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@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@momjian.us> http://momjian.us EnterpriseDB
http://postgres.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
Bruce Momjian <bruce@momjian.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.