[Patch] Log10 and hyperbolic functions for SQL:2016 compliance

=?UTF-8?Q?L=C3=A6titia_Avrot?= <laetitia.avrot@gmail.com> writes:

[ adding_log10_and_hyperbolic_functions_v1.patch ]

No objection to the feature, but

- Why are you using the float4-width library functions (coshf etc)
rather than the float8-width ones (cosh etc)?

- I wonder whether these library functions exist everywhere.
If they don't, what will we do about it ... throw an error?

I see that SUSv2 requires cosh() and so on, so it's quite possible
that these do exist everywhere we care about. I'd be okay with
throwing a patch onto the buildfarm to see, and adding an autoconf
test only if the buildfarm is unhappy. But we should be clear on
what we're going to do about it if that happens.

I added regression tests for the new functions, but I didn't for log10
function, assuming that if log function worked, log10 will work too.

Not quite sure I believe that.

Actually, what I'd counsel is that you *not* include tests of what
these do with Inf and NaN. There is no upside to doing so, and lots
of downside if older platforms are squirrely in their behavior, which
is hardly unlikely (cf opossum ...)

regards, tom lane

Hi,

Thanks for your time and advice, Tom!

[ adding_log10_and_hyperbolic_functions_v1.patch ]

No objection to the feature, but

- Why are you using the float4-width library functions (coshf etc)
rather than the float8-width ones (cosh etc)?

Well, I guess the only reason is that I wasn't paying attention enough...

I changed for the float8-width library functions.

- I wonder whether these library functions exist everywhere.
If they don't, what will we do about it ... throw an error?

I see that SUSv2 requires cosh() and so on, so it's quite possible
that these do exist everywhere we care about. I'd be okay with
throwing a patch onto the buildfarm to see, and adding an autoconf
test only if the buildfarm is unhappy. But we should be clear on
what we're going to do about it if that happens.

I think I was too confident because of the "it works on my laptop"

syndrome... I don't know how to answer to this point.

I added regression tests for the new functions, but I didn't for log10
function, assuming that if log function worked, log10 will work too.

Not quite sure I believe that.

Do you mean I should also add a regression test for the new log10 function

too ?

Actually, what I'd counsel is that you *not* include tests of what
these do with Inf and NaN. There is no upside to doing so, and lots
of downside if older platforms are squirrely in their behavior, which
is hardly unlikely (cf opossum ...)

I changed the regression tests for hyperbolic functions, so it doesn't test
for Inf and NaN.

You'll find enclosed the new version of the patch.

Cheers,

Lætitia

Hi,

Thanks to Emil Iggland's kind review, I added precision on documentation
for hyperbolic functions.

I added the patch to the next commitfest.

Cheers,

Lætitia

Le dim. 27 janv. 2019 à 20:39, Lætitia Avrot <laetitia.avrot@gmail.com> a
écrit :

Hi,

Thanks for your time and advice, Tom!

[ adding_log10_and_hyperbolic_functions_v1.patch ]

No objection to the feature, but

- Why are you using the float4-width library functions (coshf etc)
rather than the float8-width ones (cosh etc)?

Well, I guess the only reason is that I wasn't paying attention enough...

I changed for the float8-width library functions.

- I wonder whether these library functions exist everywhere.
If they don't, what will we do about it ... throw an error?

I see that SUSv2 requires cosh() and so on, so it's quite possible
that these do exist everywhere we care about. I'd be okay with
throwing a patch onto the buildfarm to see, and adding an autoconf
test only if the buildfarm is unhappy. But we should be clear on
what we're going to do about it if that happens.

I think I was too confident because of the "it works on my laptop"

syndrome... I don't know how to answer to this point.

I added regression tests for the new functions, but I didn't for log10
function, assuming that if log function worked, log10 will work too.

Not quite sure I believe that.

Do you mean I should also add a regression test for the new log10

function too ?

Actually, what I'd counsel is that you *not* include tests of what
these do with Inf and NaN. There is no upside to doing so, and lots
of downside if older platforms are squirrely in their behavior, which
is hardly unlikely (cf opossum ...)

I changed the regression tests for hyperbolic functions, so it doesn't
test for Inf and NaN.

You'll find enclosed the new version of the patch.

Cheers,

Lætitia

--
*Think! Do you really need to print this email ? *
*There is no Planet B.*

On 2019-Jan-31, L�titia Avrot wrote:

Hi,

Thanks to Emil Iggland's kind review, I added precision on documentation
for hyperbolic functions.

Hello

I see that in dtanh() you set errno to 0 before calling tanh(), but 1)
you don't check for it afterwards (seems like you should be checking for
ERANGE, as well as checking the return value for isinf()), and 2) you
don't do that in dsinh() and dcosh() and I'm not quite sure I see why.
What's up with that?

Thanks,

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hi Alvaro,

Thank you so much for taking the time to review the patch and for taking
the time again to sort things
out with me this evening.

I see that in dtanh() you set errno to 0 before calling tanh(), but 1)
you don't check for it afterwards (seems like you should be checking for
ERANGE, as well as checking the return value for isinf()), and 2) you
don't do that in dsinh() and dcosh() and I'm not quite sure I see why.
What's up with that?

At the time I wrote that patch, I tried to include errno testing.Then, I
think again and
came back with the wrong idea everything would be fine.

By re-reading math.h documentation, it is now clear that the three
functions can raise a
ERANGE error.

There are two cases :
- range error due to overflow occurs.
- range error occurs due to underflow. In that case, the correct result
(after rounding) is returned. So I assume we can ignore that case.

For sinh and cosh, we can have both cases and we added support for overflow.

For tanh, the only possible case is underflow and then, the result is
correct.

We included comments to explain errno handling in those functions.

Cheers,

Lætitia

"Lætitia" == Lætitia Avrot <laetitia.avrot@gmail.com> writes:

[snip patch]

The spec doesn't require the inverse functions (asinh, acosh, atanh),
but surely there is no principled reason to omit them?

--
Andrew (irc:RhodiumToad)

Andrew Gierth <andrew@tao11.riddles.org.uk> writes:

The spec doesn't require the inverse functions (asinh, acosh, atanh),
but surely there is no principled reason to omit them?

+1 --- AFAICS, the C library has offered all six since C89.

regards, tom lane

Hi Andrew and Tom,

I considered that option before writing my patch but I refrained for 2
reasons:

- There is no consensus about how to name these functions. The standard
8000-2 goes with arsinh, arcosh and artanh,
but you will find easily arcsinh, arccosh and arctanh or even argsinh,
argcosh and argtanh. In IT, the names asinh,
acosh and atanh are commonly used too. We might implement them with
asinh, acosh and atanh names and add
aliases if SQL standard decide to add it under other names though.
- If we go with inverse hyperbolic functions, I guess we could add other
hyperbolic functions as hyperbolic cosecant,
secant and cotangent too. Then it adds the inverse hyperbolic functions
of these three functions. These six functions
are not described in math.h library. I guess it's because these functions
are quite simple to deduce from the others.

So, as you're asking that too, maybe my reasons weren't good enough. You'll
find enclosed a new version of the patch
with asinh, acosh and atanh (v5).

Then I tried for several days to implement the 6 last hyperbolic functions,
but I wasn't satisfied with the result, so I just dropped it.

Cheers,

Lætitia

Le dim. 3 févr. 2019 à 16:12, Tom Lane <tgl@sss.pgh.pa.us> a écrit :

Andrew Gierth <andrew@tao11.riddles.org.uk> writes:

The spec doesn't require the inverse functions (asinh, acosh, atanh),
but surely there is no principled reason to omit them?

+1 --- AFAICS, the C library has offered all six since C89.

regards, tom lane

--
*Think! Do you really need to print this email ? *
*There is no Planet B.*

On 12/02/2019 06:44, Lætitia Avrot wrote:

Hi Andrew and Tom,

I considered that option before writing my patch but I refrained for 2
reasons:

- There is no consensus about how to name these functions. The
standard 8000-2 goes with arsinh, arcosh and artanh,
  but you will find easily arcsinh, arccosh and arctanh or even
argsinh, argcosh and argtanh. In IT, the names asinh,
  acosh and atanh are commonly used too. We might implement them with
asinh, acosh and atanh names and add
  aliases if SQL standard decide to add it under other names though.

[...]

Le dim. 3 févr. 2019 à 16:12, Tom Lane <tgl@sss.pgh.pa.us
<mailto:tgl@sss.pgh.pa.us>> a écrit :

Andrew Gierth <andrew@tao11.riddles.org.uk
<mailto:andrew@tao11.riddles.org.uk>> writes:

The spec doesn't require the inverse functions (asinh, acosh,

atanh),

but surely there is no principled reason to omit them?

+1 --- AFAICS, the C library has offered all six since C89.

                        regards, tom lane

[...]

I can only remember coming across the asinh, acosh, and atanh forms.  In
45 years of programming.

Cheers,
Gavin

Gavin Flower <GavinFlower@archidevsys.co.nz> writes:

On 12/02/2019 06:44, Lætitia Avrot wrote:

I considered that option before writing my patch but I refrained for 2
reasons:

- There is no consensus about how to name these functions. The
standard 8000-2 goes with arsinh, arcosh and artanh,
  but you will find easily arcsinh, arccosh and arctanh or even
argsinh, argcosh and argtanh. In IT, the names asinh,
  acosh and atanh are commonly used too. We might implement them with
asinh, acosh and atanh names and add
  aliases if SQL standard decide to add it under other names though.

I can only remember coming across the asinh, acosh, and atanh forms.  In
45 years of programming.

I don't think this is a problem. Postgres has never had any hesitation
about adopting C-standard function names if there's nothing in the SQL
standard. The C standard says asinh etc, so those are the names to use.

As Lætitia says, there'd be little problem with adding aliases if someday
the SQL committee decides to add these with other spellings.

regards, tom lane

On Sun, 3 Feb 2019 at 15:12, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Andrew Gierth <andrew@tao11.riddles.org.uk> writes:

The spec doesn't require the inverse functions (asinh, acosh, atanh),
but surely there is no principled reason to omit them?

+1 --- AFAICS, the C library has offered all six since C89.

+1 for including the inverse functions. However, it looks to me like
the inverse functions are C99-specific, so they might not be available
on all supported platforms. If they're not, we may need to provide our
own implementations.

I did a bit of research and had play. It looks like it might not be
too hard to provide our own implementations, but it does take a bit of
care to avoid rounding and overflow errors. Attached are some
standalone C programs where I tested various algorithms. A decent
approach seems to be to either use log1p() (which is itself
C99-specific, hence that's the first thing I played with) or to use a
single round of Newton-Raphson to correct rounding errors, in a
similar way to how we implement cbrt() on platforms that don't have
that.

Of course that may all be moot -- those functions may in fact be
available everywhere we care about, but it was interesting to play
around with them anyway.

Regards,
Dean

Dean Rasheed <dean.a.rasheed@gmail.com> writes:

+1 for including the inverse functions. However, it looks to me like
the inverse functions are C99-specific, so they might not be available
on all supported platforms. If they're not, we may need to provide our
own implementations.

FWIW, I'm pretty sure they're available everywhere. It's true C89
doesn't mention them, but POSIX has had them for a long time. The
SUSv2 version of POSIX has them, and so does my pet dinosaur HPUX 10.20,
which has this to say about their origin:

$ man asinh
...
STANDARDS CONFORMANCE
asinh(): SVID3, XPG4.2

Windows, as usual, is a wild card, but as far as I can tell by googling
they exist in Windows too (at least recent versions).

It's definitely possible that there are substandard implementations
out there, though. Hopefully the buildfarm will alert us to any
problems.

Of course that may all be moot -- those functions may in fact be
available everywhere we care about, but it was interesting to play
around with them anyway.

Yeah, math functions are fun to play around with ... and we could end
up needing the code. We'll see.

regards, tom lane

=?UTF-8?Q?L=C3=A6titia_Avrot?= <laetitia.avrot@gmail.com> writes:

So, as you're asking that too, maybe my reasons weren't good enough. You'll
find enclosed a new version of the patch
with asinh, acosh and atanh (v5).

Pushed with some minor adjustments (mainly cleanup of the error handling).

Then I tried for several days to implement the 6 last hyperbolic functions,
but I wasn't satisfied with the result, so I just dropped it.

Yeah, I agree that sech() and so on are not worth the trouble. If they
were commonly used, they'd be in POSIX ...

regards, tom lane

Thanks, Tom !

Thank you everyone for your help and patience.

Cheers,

Lætitia

Le mar. 12 mars 2019 à 20:57, Tom Lane <tgl@sss.pgh.pa.us> a écrit :

I really appreciate the addition of tanh into core postgres.

If someone doubts it is useful: it is used as a part of math in
geographical calculations.

Say you have your cars in planar Mercator projection and want to move them
"1 second forward by this heading with this speed". sin/cos and the
distance on X/Y, but the distance must be scaled properly - and that
scaling coefficient is cosd(latitude), which you don't have directly - you
have it in projected meters. If you don't want to fire up full-featured
PostGIS on this hot path you inline all formulas together, result is nice
and small - but has tanh in it, which I was surprised to find only in
Oracle Compatibility extensions. Pure sql tanh was good enough, but gave me
disturbance :)

Here's the code:
https://github.com/gojuno/lostgis/blob/master/sql/functions/coslat.sql#L21

On Wed, Mar 13, 2019 at 5:34 PM Lætitia Avrot <laetitia.avrot@gmail.com>
wrote:

Thanks, Tom !

Thank you everyone for your help and patience.

Cheers,

Lætitia

Le mar. 12 mars 2019 à 20:57, Tom Lane <tgl@sss.pgh.pa.us> a écrit :

=?UTF-8?Q?L=C3=A6titia_Avrot?= <laetitia.avrot@gmail.com> writes:

So, as you're asking that too, maybe my reasons weren't good enough.

You'll

find enclosed a new version of the patch
with asinh, acosh and atanh (v5).

Pushed with some minor adjustments (mainly cleanup of the error handling).

Then I tried for several days to implement the 6 last hyperbolic

functions,

but I wasn't satisfied with the result, so I just dropped it.

Yeah, I agree that sech() and so on are not worth the trouble. If they
were commonly used, they'd be in POSIX ...

regards, tom lane

--
Darafei Praliaskouski
Support me: http://patreon.com/komzpa