Re: Proposal: Trigonometric functions in degrees

Currently PostgreSQL only has trigonometric functions that work in
radians. I think it would be quite useful to have an equivalent set of
functions that worked in degrees. In other environments these are
commonly spelled sind(), cosd(), etc.

Partly, this would be a matter of convenience. It's quite common to
have a problem domain where angles are specified in degrees, and it's
somewhat cumbersome having to type things like sin(radians(x)) and
degrees(asin(x)).

Additionally, functions that worked natively in degrees would be able
to return exact answers in special cases like cosd(90) = 0, whereas
cos(radians(90)) is not exactly 0 because pi/2 cannot be represented
exactly as a floating point number.

Possibly the earthdistance module would benefit from these functions too.

Thoughts?

Regards,
Dean

On 24 October 2015 at 05:24, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:

> Currently PostgreSQL only has trigonometric functions that work in
> radians. I think it would be quite useful to have an equivalent set of
> functions that worked in degrees. In other environments these are
> commonly spelled sind(), cosd(), etc.
>
> Partly, this would be a matter of convenience. It's quite common to
> have a problem domain where angles are specified in degrees, and it's
> somewhat cumbersome having to type things like sin(radians(x)) and
> degrees(asin(x)).
>
> Additionally, functions that worked natively in degrees would be able
> to return exact answers in special cases like cosd(90) = 0, whereas
> cos(radians(90)) is not exactly 0 because pi/2 cannot be represented
> exactly as a floating point number.
>

That is important.

> Possibly the earthdistance module would benefit from these functions too.
>
> Thoughts?
>

+1, yes, please.

--
Simon Riggs http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/>
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

> Currently PostgreSQL only has trigonometric functions that work in
> radians. I think it would be quite useful to have an equivalent set of
> functions that worked in degrees. In other environments these are
> commonly spelled sind(), cosd(), etc.

I would prefer gradian over degree.

On Sun, Oct 25, 2015 at 6:16 PM, Emre Hasegeli <emre(at)hasegeli(dot)com> wrote:
>> Currently PostgreSQL only has trigonometric functions that work in
>> radians. I think it would be quite useful to have an equivalent set of
>> functions that worked in degrees. In other environments these are
>> commonly spelled sind(), cosd(), etc.
>
> I would prefer gradian over degree.

This may be a matter of personal taste, but I am personally used to
degrees since primary school. By the way, +1 for the proposal.
--
Michael

On Sun, Oct 25, 2015 at 10:16:41AM +0100, Emre Hasegeli wrote:
> > Currently PostgreSQL only has trigonometric functions that work in
> > radians. I think it would be quite useful to have an equivalent set of
> > functions that worked in degrees. In other environments these are
> > commonly spelled sind(), cosd(), etc.
>
> I would prefer gradian over degree.

We can have both, but degree is a good bit better known, which means
more users will care about it.

People have gone a long way toward dealing with problems like the
known correspondence (90° = π/2, etc.) and periodicity (f(x) =
f(x+360*n) for integer n, f in (sin, cos, tan, cot, sec, csc), for
example).

I haven't yet found same in a PostgreSQL-compatible library, though.

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On 25 October 2015 at 09:16, Emre Hasegeli <emre(at)hasegeli(dot)com> wrote:
>> Currently PostgreSQL only has trigonometric functions that work in
>> radians. I think it would be quite useful to have an equivalent set of
>> functions that worked in degrees. In other environments these are
>> commonly spelled sind(), cosd(), etc.
>
> I would prefer gradian over degree.

I think gradians are generally less commonly used than degrees and
radians, so I'm less inclined to include them.

Having degree-based functions would make it trivial to implement
user-defined gradian-based functions, just by multiplying or dividing
by 0.9, and they would return exact results in the smaller number of
cases where gradian results are exactly representable.

Regards,
Dean

Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> writes:
> On 25 October 2015 at 09:16, Emre Hasegeli <emre(at)hasegeli(dot)com> wrote:
>> I would prefer gradian over degree.

> I think gradians are generally less commonly used than degrees and
> radians, so I'm less inclined to include them.

I agree. gradians are not often used at all, AFAICT.

> Having degree-based functions would make it trivial to implement
> user-defined gradian-based functions, just by multiplying or dividing
> by 0.9, and they would return exact results in the smaller number of
> cases where gradian results are exactly representable.

... but having said that, your argument here is faulty, because 0.9
in itself is not exactly representable in binary. You'd be relying
on roundoff happening in the right direction to get exact answers
from such calculations, for just the same reasons that sind() can't be
just "sin(x * scalefactor)" if you want exact-where-possible results.

regards, tom lane

On 26 October 2015 at 10:18, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> writes:
> > On 25 October 2015 at 09:16, Emre Hasegeli <emre(at)hasegeli(dot)com> wrote:
> >> I would prefer gradian over degree.
>
> > I think gradians are generally less commonly used than degrees and
> > radians, so I'm less inclined to include them.
>
> I agree. gradians are not often used at all, AFAICT.

I've never seen anyone use a gradian, even though my calculator had them
when I was 16.

I even misread the request, thinking he meant "radians". Definitely -1 to
gradians in PostgreSQL.

Also -1 to furlongs, fortnights, pecks and hundredweight, amongst others.

--
Simon Riggs http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/>
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Mon, Oct 26, 2015 at 1:29 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On 26 October 2015 at 10:18, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> writes:
>> > On 25 October 2015 at 09:16, Emre Hasegeli <emre(at)hasegeli(dot)com> wrote:
>> >> I would prefer gradian over degree.
>>
>> > I think gradians are generally less commonly used than degrees and
>> > radians, so I'm less inclined to include them.
>>
>> I agree. gradians are not often used at all, AFAICT.
>
>
> I've never seen anyone use a gradian, even though my calculator had them
> when I was 16.
>
> I even misread the request, thinking he meant "radians". Definitely -1 to
> gradians in PostgreSQL.
>
> Also -1 to furlongs, fortnights, pecks and hundredweight, amongst others.

Aw, you're no fun. select '1 fortnight'::interval => '14 days' would be cool.

https://en.wikipedia.org/wiki/FFF_system

I don't think we should be dismissive of gradians, because I'm sure
Emre's request was serious and in good faith, but I don't feel a
crying need for them either.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 26 October 2015 at 14:18, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Having degree-based functions would make it trivial to implement
>> user-defined gradian-based functions, just by multiplying or dividing
>> by 0.9, and they would return exact results in the smaller number of
>> cases where gradian results are exactly representable.
>
> ... but having said that, your argument here is faulty, because 0.9
> in itself is not exactly representable in binary. You'd be relying
> on roundoff happening in the right direction to get exact answers
> from such calculations, for just the same reasons that sind() can't be
> just "sin(x * scalefactor)" if you want exact-where-possible results.
>

It would be relying on the things like the following being exactly true:

45 / 0.9 = 50
50 * 0.9 = 45
90 / 0.9 = 100
100 * 0.9 = 90

which they are on my machine. I thought that this was guaranteed in
IEEE floating point arithmetic, since one of the inputs and the result
are exactly representable, and the result is guaranteed to be within
0.5ulp. I'm curious now though. Are there any platforms on which the
above aren't exactly true?

Regards,
Dean

On 26 October 2015 at 13:58, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> > Also -1 to furlongs, fortnights, pecks and hundredweight, amongst others.
>
> Aw, you're no fun. select '1 fortnight'::interval => '14 days' would be
> cool.
>

Deviation from SI units is no laughing matter. How would we even know how
to capitalise "Fortnight"?

--
Simon Riggs http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/>
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> writes:
> On 26 October 2015 at 14:18, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> ... but having said that, your argument here is faulty, because 0.9
>> in itself is not exactly representable in binary. You'd be relying
>> on roundoff happening in the right direction to get exact answers
>> from such calculations, for just the same reasons that sind() can't be
>> just "sin(x * scalefactor)" if you want exact-where-possible results.

> It would be relying on the things like the following being exactly true:

> 45 / 0.9 = 50
> 50 * 0.9 = 45
> 90 / 0.9 = 100
> 100 * 0.9 = 90

> which they are on my machine. I thought that this was guaranteed in
> IEEE floating point arithmetic, since one of the inputs and the result
> are exactly representable, and the result is guaranteed to be within
> 0.5ulp. I'm curious now though. Are there any platforms on which the
> above aren't exactly true?

Possibly, but the bigger picture is you're ignoring other cases, such as

regression=# select 30 / 0.9;
?column?
---------------------
33.3333333333333333
(1 row)

which is problematic since sin(30 degrees) = 0.5 is one of the cases
one would like to be exact. (Actually I guess this example shows that
implementing sind in terms of sing would be imprecise, but I'm sure
the reverse holds for some other special angles.)

regards, tom lane

On 26 October 2015 at 18:58, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> writes:
>> On 26 October 2015 at 14:18, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> ... but having said that, your argument here is faulty, because 0.9
>>> in itself is not exactly representable in binary. You'd be relying
>>> on roundoff happening in the right direction to get exact answers
>>> from such calculations, for just the same reasons that sind() can't be
>>> just "sin(x * scalefactor)" if you want exact-where-possible results.
>
>> It would be relying on the things like the following being exactly true:
>
>> 45 / 0.9 = 50
>> 50 * 0.9 = 45
>> 90 / 0.9 = 100
>> 100 * 0.9 = 90
>
>> which they are on my machine. I thought that this was guaranteed in
>> IEEE floating point arithmetic, since one of the inputs and the result
>> are exactly representable, and the result is guaranteed to be within
>> 0.5ulp. I'm curious now though. Are there any platforms on which the
>> above aren't exactly true?
>
> Possibly, but the bigger picture is you're ignoring other cases, such as
>
> regression=# select 30 / 0.9;
> ?column?
> ---------------------
> 33.3333333333333333
> (1 row)
>
> which is problematic since sin(30 degrees) = 0.5 is one of the cases
> one would like to be exact. (Actually I guess this example shows that
> implementing sind in terms of sing would be imprecise, but I'm sure
> the reverse holds for some other special angles.)
>

No, actually sing() and the other gradian-based trig functions have
fewer exact special cases than their degree-based equivalents. That's
one of the criticisms of gradians. For example an equilateral triangle
has internal angles of 66.666... gradians. The only exact results for
sing() and cosg() are at multiples of 100 gradians.

Regards,
Dean

On 10/24/15 5:24 AM, Dean Rasheed wrote:
> Additionally, functions that worked natively in degrees would be able
> to return exact answers in special cases like cosd(90) = 0, whereas
> cos(radians(90)) is not exactly 0 because pi/2 cannot be represented
> exactly as a floating point number.

But how you are going to implement that? I don't see a sind() in the C
library.

On 26 October 2015 at 19:45, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> On 10/24/15 5:24 AM, Dean Rasheed wrote:
>> Additionally, functions that worked natively in degrees would be able
>> to return exact answers in special cases like cosd(90) = 0, whereas
>> cos(radians(90)) is not exactly 0 because pi/2 cannot be represented
>> exactly as a floating point number.
>
> But how you are going to implement that? I don't see a sind() in the C
> library.
>

I'm thinking something along the lines of:

1. Reduce the range of the input (say to 0..90 degrees).
2. Handle special cases (0, 30 and 90 for sind()).
3. Otherwise convert to radians for the general case.

Regards,
Dean

Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> writes:
> On 26 October 2015 at 19:45, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
>> But how you are going to implement that? I don't see a sind() in the C
>> library.

> I'm thinking something along the lines of:

> 1. Reduce the range of the input (say to 0..90 degrees).
> 2. Handle special cases (0, 30 and 90 for sind()).
> 3. Otherwise convert to radians for the general case.

I'd be okay with #1 and #3, but #2 is just a crock; it would risk creating
problems that did not exist before, such as non-monotonicity of the
function result (over ranges where that does not hold).

I looked into my dusty old copy of Cody & Waite's "Software Manual for the
Elementary Functions", which is what I used as a reference the last time
I had to do code like this (which admittedly was quite a long time ago;
the state of the art might've advanced). C&W say that the key accuracy
limit for sin and cos is reduction of the argument modulo pi (or whichever
multiple of pi you choose to work with). Now that problem just goes away
for degrees, of course, so it might be that reduction mod 360 and then
conversion to radians would be Good Enough(TM).

If it's not good enough, a possible idea is reduction mod 45 degrees or
even mod 30 degrees and then using trig identities to reconstruct the
correct output.

Anyway, I think the core idea of trying to build a reasonably thin wrapper
around sin(3m) and friends is probably an appropriate amount of effort,
depending on how many cases you are hoping to make exact. I doubt it's
worth coding sind() from scratch.

regards, tom lane

On 26 October 2015 at 20:19, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> writes:
>> I'm thinking something along the lines of:
>
>> 1. Reduce the range of the input (say to 0..90 degrees).
>> 2. Handle special cases (0, 30 and 90 for sind()).
>> 3. Otherwise convert to radians for the general case.
>
> I'd be okay with #1 and #3, but #2 is just a crock; it would risk creating
> problems that did not exist before, such as non-monotonicity of the
> function result (over ranges where that does not hold).
>

Well special-casing 0 and 90 are certainly no problem, and do not risk
introducing non-monotonicity. sin() changes sign at 0 and for very
small x, sin(x) ~= x, so special-casing sin(0) = 0, doesn't affect the
monotonic nature of the function in that vicinity. Near 90, sin(x) is
not monotonic, but rather that is a maxima, and the slope is 0, so
small perturbations in x do not affect the result and you have to move
quite far away from 90 for the result to become different from 1, so
special-casing sind(90) = 1 does not affect the continuity of the
function.

Special-casing sind(30) = 0.5 could be more problematic, but a quick
test on my machine shows that it does remain monotonic. As I
understand it, most modern processors apply argument reduction to the
range 0..pi/4, using cos(pi/2-x)=sin(x) to extend this to the range
0..pi/2 and hence all inputs. So passing in an input close to pi/6 (30
degrees) will not undergo any further argument reduction and the
result would be expected to be very accurate. Of course, we could just
omit this special case, and accept whatever approximation to 0.5 the
underlying function returned. It's a question of which is worse in
practice -- a small chance that sind(29.999...) or sind(30.000...1)
might be the wrong side of 0.5, making the function non-monotonic, or
having sind(30) not be exactly 0.5.

Personally I'd rather have sind(30) be exactly 0.5, even if
sind(29.999...) or sind(30.000...1) ended up the wrong side of it.
Floating point arithmetic is inherently imprecise, so tiny errors are
to be expected. But the well-known exact cases ought to work exactly.
BTW, these are the *only* values for which sind() returns exact
answers (see Niven's Theorum -
http://mathworld.wolfram.com/NivensTheorem.html) and they also happen
to be exactly representable using floats, so it seems a shame not to
return the exact values whenever possible.

> I looked into my dusty old copy of Cody & Waite's "Software Manual for the
> Elementary Functions", which is what I used as a reference the last time
> I had to do code like this (which admittedly was quite a long time ago;
> the state of the art might've advanced). C&W say that the key accuracy
> limit for sin and cos is reduction of the argument modulo pi (or whichever
> multiple of pi you choose to work with). Now that problem just goes away
> for degrees, of course, so it might be that reduction mod 360 and then
> conversion to radians would be Good Enough(TM).
>

No that would lead to sind(180) being non-zero.

> If it's not good enough, a possible idea is reduction mod 45 degrees or
> even mod 30 degrees and then using trig identities to reconstruct the
> correct output.
>

Reduction mod 45 would be fairly trivial, and would result in things
like sind(10) being exactly equal to cosd(80), even though neither
answer would be exactly correct. I'm not sure that by itself is worth
the extra effort, but it would probably improve the chances of cosd()
staying monotonic near 60, if we special-cased cosd(60) = 0.5
(although it does anyway on my machine).

Regards,
Dean

>
> ​
>

Stupid question - is sin(3m) a call-through to the math coprocessor?​ It
probably only matters when doing a series of calculations (where the extra
guard bits can matter) and not when doing a simple one-time lookup but it
might be something to consider in regards to setting a precedent.

Bear

On 10/26/15 1:48 PM, Simon Riggs wrote:
> Deviation from SI units is no laughing matter. How would we even know
> how to capitalise "Fortnight"?

It's all fun and games until someone pokes Mars' eye out with a spacecraft.

https://en.wikipedia.org/wiki/Mars_Climate_Orbiter

(Ok, I guess we just sprayed the planet with debris instead of poking an
eye out...)
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com

Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> writes:
> Personally I'd rather have sind(30) be exactly 0.5, even if
> sind(29.999...) or sind(30.000...1) ended up the wrong side of it.

TBH, sir, if you think that you are too dangerous to be allowed anywhere
near any numerical analysis code. Preserving mathematical properties like
monotonicity is *far* more important than whether sin(30 degrees) comes
out exact or not. You can do proofs about the behavior of algorithms
using these functions if you can ensure monotonicity; but exactness of
values is always going to depend on subtle details of the floating point
format.

I think using a range reduction to some fractional part of the circle is a
reasonable way of trying to deal with these concerns, but I really really
do not want to see it special-casing point values in a way that doesn't
guarantee monotonicity.

It may be that guaranteeing the sin(30) case is just not very feasible,
and we should content ourselves with getting the 0/90/180/270/360 cases
exactly, which we could do with a mod 90 initial reduction. Doing mod 30
or mod 45 would require sewing together pieces of the curve that might not
meet exactly, if we were unlucky. (I've not tried it though.)

What have you got in mind for tan() and the rest?

regards, tom lane

On 26 October 2015 at 22:51, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> writes:
>> Personally I'd rather have sind(30) be exactly 0.5, even if
>> sind(29.999...) or sind(30.000...1) ended up the wrong side of it.
>
> TBH, sir, if you think that you are too dangerous to be allowed anywhere
> near any numerical analysis code. Preserving mathematical properties like
> monotonicity is *far* more important than whether sin(30 degrees) comes
> out exact or not. You can do proofs about the behavior of algorithms
> using these functions if you can ensure monotonicity; but exactness of
> values is always going to depend on subtle details of the floating point
> format.
>
> I think using a range reduction to some fractional part of the circle is a
> reasonable way of trying to deal with these concerns, but I really really
> do not want to see it special-casing point values in a way that doesn't
> guarantee monotonicity.
>

OK, agreed. Preserving the monotonicity of sind over the range 0..90
is important. It's useful having this discussion before implementing a
lot of code that people might come to rely on.

> It may be that guaranteeing the sin(30) case is just not very feasible,
> and we should content ourselves with getting the 0/90/180/270/360 cases
> exactly, which we could do with a mod 90 initial reduction. Doing mod 30
> or mod 45 would require sewing together pieces of the curve that might not
> meet exactly, if we were unlucky. (I've not tried it though.)
>

I think it's still feasible to have sind(30) = 0.5 exactly and keep
monotonicity. If we can implement sind over the range 0..90, the rest
is easy, and in that range there are 3 known fixed points that we want
to preserve -- (0,0), (30,0.5) and (90,1). So all we need is a way to
compute each half of the range, interpolating between the fixed
points. That ought to be possible by pre-computing the value of
sin(pi/6) and using it to scale the result of sin(x) to guarantee that
the pieces join in the middle. (I've not tried it either, but it
sounds plausible.)

> What have you got in mind for tan() and the rest?
>

If we have sind() for the range 0..90, cosd() could be implemented as
sind(90 - x) after reducing x to the range 0..90, which would then
give it the right set of properties and it would be exact for 0, 60
and 90.

Then tand() and cotd() could be implemented as their ratios, which
would guarantee that tand(45) = cotd(45) = 1 exactly and that they are
monotonic nearby, without needing any special case code.

I haven't thought about the inverse functions yet, but they ought to
be possible in a similar way, albeit a bit fiddly.

Regards,
Dean

On Mon, Oct 26, 2015 at 01:58:52PM -0400, Robert Haas wrote:
> Aw, you're no fun. select '1 fortnight'::interval => '14 days' would be cool.

Patch attached...

:)

% psql -p 5433 -d template1 -h localhost
psql (9.4.5, server 9.6devel)
WARNING: psql major version 9.4, server major version 9.6.
Some psql features might not work.
Type "help" for help.

template1=# select current_date;
date
------------
2015-10-27
(1 row)

template1=# select '1 fortnight'::interval;
interval
----------
14 days
(1 row)

template1=# select current_date + '1 fortnight'::interval;
?column?
---------------------
2015-11-10 00:00:00
(1 row)

template1=# select current_date + '1.3 fortnight'::interval;
?column?
---------------------
2015-11-14 04:48:00
(1 row)

template1=# select current_date + '1.3 fortnights'::interval;
?column?
---------------------
2015-11-14 04:48:00
(1 row)

--
nw

On Tue, Oct 27, 2015 at 8:52 AM, Nathan Wagner <nw+pg(at)hydaspes(dot)if(dot)org> wrote:
> On Mon, Oct 26, 2015 at 01:58:52PM -0400, Robert Haas wrote:
>> Aw, you're no fun. select '1 fortnight'::interval => '14 days' would be cool.
>
> Patch attached...
>
> :)

This is very cool (you are 100% certain there are no performance
impacts on current cases, right?)! :-)

merlin

On Tue, Oct 27, 2015 at 01:52:11PM +0000, Nathan Wagner wrote:
> On Mon, Oct 26, 2015 at 01:58:52PM -0400, Robert Haas wrote:
> > Aw, you're no fun. select '1 fortnight'::interval => '14 days'
> > would be cool.
>
> Patch attached...
>
> :)

I'd argue for a back-patch all the way to 9.1, as the lack of
fortnight support is clearly a bug.

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On 28/10/15 02:52, Nathan Wagner wrote:
> On Mon, Oct 26, 2015 at 01:58:52PM -0400, Robert Haas wrote:
>> Aw, you're no fun. select '1 fortnight'::interval => '14 days' would be cool.
> Patch attached...
>
> :)
>
[...]

You trying to get PostgreSQL banned in France??? :-)

When I was learning French many years ago, I was told that the French
consider their fortnight to be 15 days!!!

see:
https://nz.answers.yahoo.com/question/index?qid=20100920093722AAc54Xo
https://en.wikipedia.org/wiki/Fortnight
http://www.wordsense.eu/fortnight

Cheers,
Gavin

On Wed, Oct 28, 2015 at 08:17:25AM +1300, Gavin Flower wrote:
> You trying to get PostgreSQL banned in France??? :-)
>
> When I was learning French many years ago, I was told that the French
> consider their fortnight to be 15 days!!!

What, it's a "fortnight", not a "quinzaine".

You have no idea how hard it was to resist updating the patch...

--
nw

On Tue, Oct 27, 2015 at 12:04:55PM -0500, Merlin Moncure wrote:
> On Tue, Oct 27, 2015 at 8:52 AM, Nathan Wagner <nw+pg(at)hydaspes(dot)if(dot)org> wrote:
> > On Mon, Oct 26, 2015 at 01:58:52PM -0400, Robert Haas wrote:
> >> Aw, you're no fun. select '1 fortnight'::interval => '14 days' would be cool.
> >
> > Patch attached...

> This is very cool (you are 100% certain there are no performance
> impacts on current cases, right?)! :-)

It passed the regression test. It must be perfect :)

--
nw

On Tue, Oct 27, 2015 at 01:52:11PM +0000, Nathan Wagner wrote:
> On Mon, Oct 26, 2015 at 01:58:52PM -0400, Robert Haas wrote:
> > Aw, you're no fun. select '1 fortnight'::interval => '14 days' would be cool.
>
> Patch attached...

This isn't necessarily bad, but I observe that it would be difficult or
impossible to add fortnight support to intervals as an extension rather
than by modifying the scanner tables that the interval parser uses at
original compile time. You might be able to override symbols with a C
extension, but parts of the parser are static (in the C sense), so you'd
need to override and duplicate a lot of the existing functions. Of
course, a hookable interval parser is absurd in the first place.

--
nw

On 27 October 2015 at 08:24, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
> I think it's still feasible to have sind(30) = 0.5 exactly and keep
> monotonicity....
>

Here's a patch along those lines. It turned out to be fairly
straightforward. It's still basically a thin wrapper on top of the
math library trig functions, with a bit of careful scaling to ensure
that curves join together to form continuous functions that are
monotonic in the expected regions and return exact values in all the
special cases 0,30,45,60,90,...

I also modified some of the CHECKFLOATVAL() checks which didn't look
right to me, unless there's some odd platform-specific behaviour that
I'm not aware of, functions like sin and asin should never return
infinity.

Regards,
Dean

On Tue, Oct 27, 2015 at 07:30:13PM +0000, Nathan Wagner wrote:
> On Wed, Oct 28, 2015 at 08:17:25AM +1300, Gavin Flower wrote:
> > You trying to get PostgreSQL banned in France??? :-)
> >
> > When I was learning French many years ago, I was told that the French
> > consider their fortnight to be 15 days!!!
>
> What, it's a "fortnight", not a "quinzaine".
>
> You have no idea how hard it was to resist updating the patch...

Well, if you won't do it, I will.

Please find attached.

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

David Fetter <david(at)fetter(dot)org> writes:
> On Tue, Oct 27, 2015 at 07:30:13PM +0000, Nathan Wagner wrote:
>> On Wed, Oct 28, 2015 at 08:17:25AM +1300, Gavin Flower wrote:
>> What, it's a "fortnight", not a "quinzaine".
>>
>> You have no idea how hard it was to resist updating the patch...

> Well, if you won't do it, I will.

Please tell me this is a joke.

(FWIW, I don't have a problem with "fortnight", but I draw the line
at units that are not used in English.)

regards, tom lane

On Sun, Nov 01, 2015 at 01:06:39PM -0500, Tom Lane wrote:
> David Fetter <david(at)fetter(dot)org> writes:
> > On Tue, Oct 27, 2015 at 07:30:13PM +0000, Nathan Wagner wrote:
> >> On Wed, Oct 28, 2015 at 08:17:25AM +1300, Gavin Flower wrote:
> >> What, it's a "fortnight", not a "quinzaine".
> >>
> >> You have no idea how hard it was to resist updating the patch...
>
> > Well, if you won't do it, I will.
>
> Please tell me this is a joke.

Yes, it's a joke.

> (FWIW, I don't have a problem with "fortnight", but I draw the line
> at units that are not used in English.)

It's used in English, but not in this context.

https://en.wikipedia.org/wiki/Quinzaine

As to localization, I think we need to consider carefully whether
PostgreSQL is to be a US-only RDBMS with a few concessions to usage
elsewhere, or one usable by all the world's peoples, and no, we should
probably not continue this thread to work out the decisions implicit
in whatever direction we settle on.

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On Sun, Nov 1, 2015 at 2:20 PM, David Fetter <david(at)fetter(dot)org> wrote:
> On Sun, Nov 01, 2015 at 01:06:39PM -0500, Tom Lane wrote:
>> David Fetter <david(at)fetter(dot)org> writes:
>> > On Tue, Oct 27, 2015 at 07:30:13PM +0000, Nathan Wagner wrote:
>> >> On Wed, Oct 28, 2015 at 08:17:25AM +1300, Gavin Flower wrote:
>> >> What, it's a "fortnight", not a "quinzaine".
>> >>
>> >> You have no idea how hard it was to resist updating the patch...
>>
>> > Well, if you won't do it, I will.
>>
>> Please tell me this is a joke.
>
> Yes, it's a joke.
>
>> (FWIW, I don't have a problem with "fortnight", but I draw the line
>> at units that are not used in English.)
>
> It's used in English, but not in this context.
>
> https://en.wikipedia.org/wiki/Quinzaine
>
> As to localization, I think we need to consider carefully whether
> PostgreSQL is to be a US-only RDBMS with a few concessions to usage
> elsewhere, or one usable by all the world's peoples

That baggage comes with the SQL Standard. It's fun to think about a
functional equivalent of SQL that doesn't attempt to have statements
be grammatically correct English sentences but due to various human
pressures that isn't how things worked out. Since I'm daydreaming,
let's have this hypothetical language implement relational division.

By the way, I fell for the joke. Note, we do implement 'allballs' so
I'll take a pass.

merlin

On 11/02/2015 09:20 AM, Merlin Moncure wrote:
> On Sun, Nov 1, 2015 at 2:20 PM, David Fetter <david(at)fetter(dot)org> wrote:
>> On Sun, Nov 01, 2015 at 01:06:39PM -0500, Tom Lane wrote:
>>> David Fetter <david(at)fetter(dot)org> writes:
>>>> On Tue, Oct 27, 2015 at 07:30:13PM +0000, Nathan Wagner wrote:
>>>>> On Wed, Oct 28, 2015 at 08:17:25AM +1300, Gavin Flower wrote:
>>>>> What, it's a "fortnight", not a "quinzaine".
>>>>>
>>>>> You have no idea how hard it was to resist updating the patch...
>>>> Well, if you won't do it, I will.
>>> Please tell me this is a joke.
>> Yes, it's a joke.
>>
>>> (FWIW, I don't have a problem with "fortnight", but I draw the line
>>> at units that are not used in English.)
>> It's used in English, but not in this context.
>>
>> https://en.wikipedia.org/wiki/Quinzaine
>>
>> As to localization, I think we need to consider carefully whether
>> PostgreSQL is to be a US-only RDBMS with a few concessions to usage
>> elsewhere, or one usable by all the world's peoples
> That baggage comes with the SQL Standard. It's fun to think about a
> functional equivalent of SQL that doesn't attempt to have statements
> be grammatically correct English sentences but due to various human
> pressures that isn't how things worked out. Since I'm daydreaming,
> let's have this hypothetical language implement relational division.
>
> By the way, I fell for the joke. Note, we do implement 'allballs' so
> I'll take a pass.
>

I respectfully submit that there is a case for supporting lovers of Jane
Austen (such as me!) by recognizing "se'nnight", with or without
apostrophe, as a synonym for "week".

cheers

andrew

On 02/11/15 07:06, Tom Lane wrote:
> David Fetter <david(at)fetter(dot)org> writes:
>> On Tue, Oct 27, 2015 at 07:30:13PM +0000, Nathan Wagner wrote:
>>> On Wed, Oct 28, 2015 at 08:17:25AM +1300, Gavin Flower wrote:

I had actually written:

> You trying to get PostgreSQL banned in France???
>
> When I was learning French many years ago, I was told that the French
> consider their fortnight to be 15 days!!!

The following 2 lines were from Nathan, not me!
>>> What, it's a "fortnight", not a "quinzaine".
>>>
>>> You have no idea how hard it was to resist updating the patch...
>> Well, if you won't do it, I will.
> Please tell me this is a joke.
>
> (FWIW, I don't have a problem with "fortnight", but I draw the line
> at units that are not used in English.)
>
> regards, tom lane
>
>

On Wed, Oct 28, 2015 at 4:17 AM, Gavin Flower wrote:
> You trying to get PostgreSQL banned in France??? :-)
>
> When I was learning French many years ago, I was told that the French
> consider their fortnight to be 15 days!!!

Confirmed. I would translate fornight as 'quinzaine' to French.
(please let's not add that btw).
--
Michael

Michael Paquier wrote:
> On Wed, Oct 28, 2015 at 4:17 AM, Gavin Flower wrote:
> > You trying to get PostgreSQL banned in France??? :-)
> >
> > When I was learning French many years ago, I was told that the French
> > consider their fortnight to be 15 days!!!
>
> Confirmed. I would translate fornight as 'quinzaine' to French.
> (please let's not add that btw).

I don't know what's the origin of the word "fortnight", in particular
whether it's always supposed to be exactly fourteen days, but the
equivalent to quizaine in Spanish ("quincena") is ambiguous enough that
I would have trouble making it part of intervals in Postgres. It is
sometimes interpreted as "two weeks", other times as exactly fifteen
days, other times as "half a month".

(WRT the reference to Jane Austen and "Se'ennight" for "week", it occurs
to me that fortnight is a similar contraction for "forteen night".)

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

On Tue, Nov 3, 2015 at 8:31 AM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote:
> (WRT the reference to Jane Austen and "Se'ennight" for "week", it occurs
> to me that fortnight is a similar contraction for "forteen night".)

Well, clearly we also need enquië for the elves of Arda and tenday for
for Faerûnians. Seems like a serious oversight, though making enquië
work in non-Unicode locales might be tricky.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Nov 4, 2015 at 2:31 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Tue, Nov 3, 2015 at 8:31 AM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote:
>> (WRT the reference to Jane Austen and "Se'ennight" for "week", it occurs
>> to me that fortnight is a similar contraction for "forteen night".)
>
> Well, clearly we also need enquië for the elves of Arda and tenday for
> for Faerûnians. Seems like a serious oversight, though making enquië
> work in non-Unicode locales might be tricky.

You are forgetting Klingon, parseltongue, dwarfic and the one of
Mordor. It's not worth angering them as well. But I'll stop here.
--
Michael

On Tue, Nov 3, 2015 at 6:40 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Wed, Nov 4, 2015 at 2:31 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Tue, Nov 3, 2015 at 8:31 AM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote:
>>> (WRT the reference to Jane Austen and "Se'ennight" for "week", it occurs
>>> to me that fortnight is a similar contraction for "forteen night".)
>>
>> Well, clearly we also need enquië for the elves of Arda and tenday for
>> for Faerûnians. Seems like a serious oversight, though making enquië
>> work in non-Unicode locales might be tricky.
>
> You are forgetting Klingon, parseltongue, dwarfic and the one of
> Mordor. It's not worth angering them as well. But I'll stop here.

I'm not aware that any of those have weeks of some duration other than
7 days, but the lack of support for the Klingon calendar in general is
indeed troubling.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Sun, Nov 1, 2015 at 9:34 PM, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
> On 27 October 2015 at 08:24, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
>> I think it's still feasible to have sind(30) = 0.5 exactly and keep
>> monotonicity....
>>
>
> Here's a patch along those lines. It turned out to be fairly
> straightforward. It's still basically a thin wrapper on top of the
> math library trig functions, with a bit of careful scaling to ensure
> that curves join together to form continuous functions that are
> monotonic in the expected regions and return exact values in all the
> special cases 0,30,45,60,90,...
>
> I also modified some of the CHECKFLOATVAL() checks which didn't look
> right to me, unless there's some odd platform-specific behaviour that
> I'm not aware of, functions like sin and asin should never return
> infinity.

The alternative expected outputs for test float8 need to be updated,
this is causing regression failures particularly on win32 where 3
digits are used for exponentials and where tan('NaN') actually results
in an ERROR. See for example the attached regressions.diffs.
--
Michael

On Tue, Nov 10, 2015 at 11:17 PM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
wrote:
> On Sun, Nov 1, 2015 at 9:34 PM, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com>
wrote:
>> On 27 October 2015 at 08:24, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com>
wrote:
>>> I think it's still feasible to have sind(30) = 0.5 exactly and keep
>>> monotonicity....
>>>
>>
>> Here's a patch along those lines. It turned out to be fairly
>> straightforward. It's still basically a thin wrapper on top of the
>> math library trig functions, with a bit of careful scaling to ensure
>> that curves join together to form continuous functions that are
>> monotonic in the expected regions and return exact values in all the
>> special cases 0,30,45,60,90,...
>>
>> I also modified some of the CHECKFLOATVAL() checks which didn't look
>> right to me, unless there's some odd platform-specific behaviour that
>> I'm not aware of, functions like sin and asin should never return
>> infinity.

- CHECKFLOATVAL(result, isinf(arg1), true);
+ CHECKFLOATVAL(result, false, true);
PG_RETURN_FLOAT8(result);

Hm. I would let them as-is, and update your patch to do the similar checks
in the new functions introduced. See f9ac414 from 2007 which is the result
of the following thread:
http://www.postgresql.org/message-id/200612271844.kBRIiVb18465@momjian.us
It doesn't seem wise to be backward regarding those Inf/NaN checks.

> The alternative expected outputs for test float8 need to be updated,
> this is causing regression failures particularly on win32 where 3
> digits are used for exponentials and where tan('NaN') actually results
> in an ERROR. See for example the attached regressions.diffs.

It would be nice to split the results specific to NaN and Infinity into
separate queries. The test for arctangent is one where things should be
splitted.

c5e86ea took some of the OIDs of this previous patch, so I rebased it as
attached.

result = 1.0 / result;
- CHECKFLOATVAL(result, true /* cotan(pi/2) == inf */ , true);
+ CHECKFLOATVAL(result, true /* cot(0) == Inf */ , true);
PG_RETURN_FLOAT8(result);
This one is true. it could be corrected as an independent fix.

+ if (isinf(arg1) || arg1 < -1 || arg1 > 1)
+ ereport(ERROR,
+
(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
+ errmsg("input is out of range")));
This should check on -1.0 and 1.0?

+ if (arg1 > 90)
+ {
+ /* tand(180-x) = -tand(x) */
+ arg1 = 180 - arg1;
+ sign = -sign;
+ }
Similarly the series of checks in atand, dcosd, asind, should use .0
precision points? Same remark for other code paths like cosd_0_to_60 for
example.

+ if (arg1 > 180)
+ {
+ /* tand(360-x) = -tand(x) */
+ arg1 = 360 - arg1;
+ sign = -sign;
+ }
Picky detail: be careful of incorrect tab spaces.

=# select acos(-1.1);
acos
------
NaN
(1 row)
=# select acosd(-1.1);
ERROR: 22003: input is out of range
LOCATION: dacosd, float.c:1752
Some results are inconsistent, it seems that we should return NaN in the
second case instead of an error.

I had as well a look at the monotony of those functions, using rough
queries like this one, and things are looking nice. The precise values are
taken into account and their surroundings are monotone.
with degrees as (
select generate_series(89.999999998, 90.000000002, 0.000000001)
union all select generate_series(44.999999998, 45.000000002, 0.000000001)
union all select generate_series(29.999999998, 30.000000002, 0.000000001)
union all select generate_series(-0.000000002, 0.000000002, 0.000000001)
union all select generate_series(59.999999998, 60.000000002, 0.000000001))
SELECT x, cosd(x), sind(x), tand(x) FROM degrees as deg(x);
with degrees as (
select generate_series((sqrt(3) / 3 - 0.00001)::numeric, (sqrt(3) / 3 +
0.00001)::numeric, 0.000001)
union all select generate_series((sqrt(3) / 2 - 0.00001)::numeric, (sqrt(3)
/ 2 + 0.00001)::numeric, 0.000001)
union all select generate_series(0.5 - 0.00001, 0.5 + 0.00001, 0.000001)
union all select generate_series(0, 0.00001, 0.000001)
union all select generate_series(0.99999, 1, 0.000001))
select x, acosd(x), asind(x), atand(x) from degrees as deg(x);
Attached are the results of all those things if others want to have a look.
Regards,
--
Michael

On 11 November 2015 at 06:04, Michael Paquier <michael(dot)paquier(at)gmail(dot)com> wrote:
>>> I also modified some of the CHECKFLOATVAL() checks which didn't look
>>> right to me, unless there's some odd platform-specific behaviour that
>>> I'm not aware of, functions like sin and asin should never return
>>> infinity.
>
> - CHECKFLOATVAL(result, isinf(arg1), true);
> + CHECKFLOATVAL(result, false, true);
> PG_RETURN_FLOAT8(result);
>
> Hm. I would let them as-is, and update your patch to do the similar checks
> in the new functions introduced. See f9ac414 from 2007 which is the result
> of the following thread:
> http://www.postgresql.org/message-id/200612271844.kBRIiVb18465@momjian.us
> It doesn't seem wise to be backward regarding those Inf/NaN checks.
>

The conclusion of that thread seemed to be that we ought to allow
silent underflows to 0, but any finite inputs that produce infinite
results ought to consider reporting an error.

That seems like a reasonable principle, but it's not what the code
actually does. For example, 1e-300::float8 * 1e-300::float8 generates
an error rather than silently underflowing to 0. In addition, some of
the checks appear to be backwards, for example the division functions
like float4div do the following:

CHECKFLOATVAL(result, isinf(arg1) || isinf(arg2), arg1 == 0);

whereas the result is 0 not infinity if arg2 is infinite (unless arg1
is also infinite, in which case it ought to be NaN).

In the case of functions like sin(), I can well believe that there are
some platforms for which sin(Infinity) is NaN, and others for which it
is an error, but are there really any for which the result is
infinite? If so, I'd argue that we should throw an error anyway --
sin(x) is supposed to be between -1 and 1, so I don't think allowing
an infinite result ever makes sense.

Anyway, this looks like a wider discussion than the scope of this
patch, so I'll revert those changes in this patch, and the decision
about what (if anything) should be done with those CHECKFLOATVAL
checks can be discussed separately.

>> The alternative expected outputs for test float8 need to be updated,
>> this is causing regression failures particularly on win32 where 3
>> digits are used for exponentials and where tan('NaN') actually results
>> in an ERROR. See for example the attached regressions.diffs.
>
> It would be nice to split the results specific to NaN and Infinity into
> separate queries. The test for arctangent is one where things should be
> splitted.
>

Agreed. In fact, given the platform-dependent nature of those tests,
perhaps there's not much value in them at all.

> + if (isinf(arg1) || arg1 < -1 || arg1 > 1)
> + ereport(ERROR,
> +
> (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
> + errmsg("input is out of range")));
> This should check on -1.0 and 1.0?
>

Perhaps. I admit that I'm not terribly consistent when I write such
code and sometimes I just rely on implicit type promotion, and other
times I prefer to be explicit. Is there a project recommended style on
this? The current code seems to be somewhat inconsistent (e.g., dpow,
dlog1, dlog10 and setseed all have similar comparisons of doubles with
1, whereas float8_regr_syy, etc. compare against 1.0). I don't have
any particular preference, so I'll happily go with whatever other
people prefer, if there's a clear consensus.

> + if (arg1 > 180)
> + {
> + /* tand(360-x) = -tand(x) */
> + arg1 = 360 - arg1;
> + sign = -sign;
> + }
> Picky detail: be careful of incorrect tab spaces.
>

Oops. Will fix.

> =# select acos(-1.1);
> acos
> ------
> NaN
> (1 row)

I get an error for that, so it's clearly platform-dependent.

> =# select acosd(-1.1);
> ERROR: 22003: input is out of range
> LOCATION: dacosd, float.c:1752
> Some results are inconsistent, it seems that we should return NaN in the
> second case instead of an error.
>

I opted to have acosd() throw the same error that acos() does on my
platform, and I tend to think an error is more useful in this case.
Perhaps if consistency is important, we should modify the existing
functions to throw an error on all platforms, rather than being
platform-dependent.

> I had as well a look at the monotony of those functions, using rough queries
> like this one, and things are looking nice. The precise values are taken
> into account and their surroundings are monotone.

Thanks for testing. I'll post an updated patch sometime soon.

Regards,
Dean

On 11 November 2015 at 11:45, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
> Thanks for testing. I'll post an updated patch sometime soon.
>

I finally got round to looking at this again, and here is an updated patch.

I've reverted the changes to the CHECKFLOATVAL() checks in the
existing functions, so that can be a separate discussion. As for the
checks in the new functions added by this patch, I've left them as
they were on the grounds that the checks are taking place after
argument reduction, so the arguments will not be infinite at that
point (and besides, I think the checks are correct as written anyway).

I've reduced the regression tests down to checks of the cases where
the results should be exact, so they should now be
platform-independent. Many of the original tests were useful during
development to ensure that sane-looking answers were being returned,
but they didn't really add anything as regression tests, other than
extra complication due to platform variations.

Regards,
Dean

On Fri, Nov 27, 2015 at 6:33 PM, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
>
> On 11 November 2015 at 11:45, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
> > Thanks for testing. I'll post an updated patch sometime soon.
> >
>
> I finally got round to looking at this again, and here is an updated patch.

Cool, thanks!

> I've reverted the changes to the CHECKFLOATVAL() checks in the
> existing functions, so that can be a separate discussion. As for the
> checks in the new functions added by this patch, I've left them as
> they were on the grounds that the checks are taking place after
> argument reduction, so the arguments will not be infinite at that
> point (and besides, I think the checks are correct as written anyway).

On this one, I agree less for the new node but I am fine to let the
committer take the final shot. Making things similar to the existing
functions seems the best approach to me though.

> I've reduced the regression tests down to checks of the cases where
> the results should be exact, so they should now be
> platform-independent. Many of the original tests were useful during
> development to ensure that sane-looking answers were being returned,
> but they didn't really add anything as regression tests, other than
> extra complication due to platform variations.

That's definitely a better thing to do. I got surprised as well for
example by how things behave differently on OSX, Linux and Windows
when testing your patch :)

+ * Stitch together inverse sine and cosine functions for the ranges
+ * [0, 0.5] and [0.5, 1]. Each expression below is guaranteed to return
+ * exactly 30 for x=0.5, so the result is a continuous
monotonic function
+ * over the full range.

+ * Stitch together inverse sine and cosine functions for the ranges
+ * [0, 0.5] and [0.5, 1]. Each expression below is guaranteed to return
+ * exactly 60 for x=0.5, so the result is a continuous
monotonic function
+ * over the full range.

Those two should mention [0,0.5] and (0.5,1]. 0.5 is only part of the
first portion. There are a couple of places where that's not exact as
well, but no real big deal.

Now on OSX the following things are inconsistent:
1) acos:
=# select acosd(1.1);
ERROR: 22003: input is out of range
LOCATION: dacosd, float.c:1752
Time: 0.623 ms
=# select acos(1.1);
acos
------
NaN
(1 row)
=# select asind('Infinity'::float8);
ERROR: 22003: input is out of range
LOCATION: dasind, float.c:1778
2) asin:
=# select asind(1.1);
ERROR: 22003: input is out of range
LOCATION: dasind, float.c:1778
=# select asin(1.1);
asin
------
NaN
(1 row)
Instinctively, it seems to me that we had better return Nan for the
new asind and acosd when being out of range for OSX, Linux will
complain about an out-of-range error so the code is right in this
case.

3) those ones as well are interesting:
=# select tand(180);
tand
------
-0
(1 row)
=# select cotd(-90);
cotd
------
-0

Regards,
--
Michael

On Mon, Nov 30, 2015 at 10:36 PM, Michael Paquier wrote:
> Instinctively, it seems to me that we had better return Nan for the
> new asind and acosd when being out of range for OSX, Linux will
> complain about an out-of-range error so the code is right in this
> case.

This is still mentioned upthread btw. And it does not seem to be a
good idea to change this platform dependent behavior by throwing an
error on the old functions, neither does it seem user-friendly to have
inconsistent results for the XXX function and its XXXd equivalent.
--
Michael

On Mon, Nov 30, 2015 at 10:39 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Mon, Nov 30, 2015 at 10:36 PM, Michael Paquier wrote:
>> Instinctively, it seems to me that we had better return Nan for the
>> new asind and acosd when being out of range for OSX, Linux will
>> complain about an out-of-range error so the code is right in this
>> case.
>
> This is still mentioned upthread btw. And it does not seem to be a
> good idea to change this platform dependent behavior by throwing an
> error on the old functions, neither does it seem user-friendly to have
> inconsistent results for the XXX function and its XXXd equivalent.

At this stage, it seems wiser to me to mark this patch as "returned
with feedback". Other opinions of course are welcome.
--
Michael

Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
> On Mon, Nov 30, 2015 at 10:36 PM, Michael Paquier wrote:
>> Instinctively, it seems to me that we had better return Nan for the
>> new asind and acosd when being out of range for OSX, Linux will
>> complain about an out-of-range error so the code is right in this
>> case.

> This is still mentioned upthread btw. And it does not seem to be a
> good idea to change this platform dependent behavior by throwing an
> error on the old functions, neither does it seem user-friendly to have
> inconsistent results for the XXX function and its XXXd equivalent.

FWIW, I think that probably the best course of action is to go ahead
and install POSIX-compliant error checking in the existing functions
too. POSIX:2008 is quite clear about this:

: An application wishing to check for error situations should set errno to
: zero and call feclearexcept(FE_ALL_EXCEPT) before calling these
: functions. On return, if errno is non-zero or fetestexcept(FE_INVALID |
: FE_DIVBYZERO | FE_OVERFLOW | FE_UNDERFLOW) is non-zero, an error has
: occurred.

Although I'm not too sure about Windows, the inconsistent results
we're getting on OS X are certainly from failure to adhere to the spec.

I concur with Peter's opinion that this is material for a separate
patch, but it seems like it's one that had better go in first.

regards, tom lane

On Mon, Nov 30, 2015 at 11:11 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
>> On Mon, Nov 30, 2015 at 10:36 PM, Michael Paquier wrote:
>>> Instinctively, it seems to me that we had better return Nan for the
>>> new asind and acosd when being out of range for OSX, Linux will
>>> complain about an out-of-range error so the code is right in this
>>> case.
>
>> This is still mentioned upthread btw. And it does not seem to be a
>> good idea to change this platform dependent behavior by throwing an
>> error on the old functions, neither does it seem user-friendly to have
>> inconsistent results for the XXX function and its XXXd equivalent.
>
> FWIW, I think that probably the best course of action is to go ahead
> and install POSIX-compliant error checking in the existing functions
> too. POSIX:2008 is quite clear about this:
>
> : An application wishing to check for error situations should set errno to
> : zero and call feclearexcept(FE_ALL_EXCEPT) before calling these
> : functions. On return, if errno is non-zero or fetestexcept(FE_INVALID |
> : FE_DIVBYZERO | FE_OVERFLOW | FE_UNDERFLOW) is non-zero, an error has
> : occurred.

OK, I have to admit I didn't know this part.

> Although I'm not too sure about Windows, the inconsistent results
> we're getting on OS X are certainly from failure to adhere to the spec.

Windows complains about out-of-range errors contrary to OSX on for
example asin or acos. So for once Linux and Windows agree with each
other.

> I concur with Peter's opinion that this is material for a separate
> patch, but it seems like it's one that had better go in first.

(I think you mean Dean here and not Peter).
--
Michael

On Mon, Nov 30, 2015 at 11:24 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Mon, Nov 30, 2015 at 11:11 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
>>> On Mon, Nov 30, 2015 at 10:36 PM, Michael Paquier wrote:
>>>> Instinctively, it seems to me that we had better return Nan for the
>>>> new asind and acosd when being out of range for OSX, Linux will
>>>> complain about an out-of-range error so the code is right in this
>>>> case.
>>
>>> This is still mentioned upthread btw. And it does not seem to be a
>>> good idea to change this platform dependent behavior by throwing an
>>> error on the old functions, neither does it seem user-friendly to have
>>> inconsistent results for the XXX function and its XXXd equivalent.
>>
>> FWIW, I think that probably the best course of action is to go ahead
>> and install POSIX-compliant error checking in the existing functions
>> too. POSIX:2008 is quite clear about this:
>>
>> : An application wishing to check for error situations should set errno to
>> : zero and call feclearexcept(FE_ALL_EXCEPT) before calling these
>> : functions. On return, if errno is non-zero or fetestexcept(FE_INVALID |
>> : FE_DIVBYZERO | FE_OVERFLOW | FE_UNDERFLOW) is non-zero, an error has
>> : occurred.
>
> OK, I have to admit I didn't know this part.
>
>> Although I'm not too sure about Windows, the inconsistent results
>> we're getting on OS X are certainly from failure to adhere to the spec.
>
> Windows complains about out-of-range errors contrary to OSX on for
> example asin or acos. So for once Linux and Windows agree with each
> other.
>
>> I concur with Peter's opinion that this is material for a separate
>> patch, but it seems like it's one that had better go in first.
>
> (I think you mean Dean here and not Peter).

Dean, are you planning to continue working on this patch? If yes, are
you fine to move it to next CF? It seems that the current consensus is
to split this effort into two patches:
1) Harden error checks for existing functions, particularly the
inconsistencies for asin and acos.
2) Have the new functions in degrees.
--
Michael

On 1 December 2015 at 12:59, Michael Paquier <michael(dot)paquier(at)gmail(dot)com> wrote:
> Dean, are you planning to continue working on this patch? If yes, are
> you fine to move it to next CF? It seems that the current consensus is
> to split this effort into two patches:

Yes, I still plan to work on it. I might not get much time over the
next few days, so moving it to the next CF is probably reasonable.

Regards,
Dean

On Wed, Dec 2, 2015 at 3:30 AM, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
> On 1 December 2015 at 12:59, Michael Paquier <michael(dot)paquier(at)gmail(dot)com> wrote:
>> Dean, are you planning to continue working on this patch? If yes, are
>> you fine to move it to next CF? It seems that the current consensus is
>> to split this effort into two patches:
>
> Yes, I still plan to work on it. I might not get much time over the
> next few days, so moving it to the next CF is probably reasonable.

Deal.
--
Michael

On 30 November 2015 at 14:11, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> FWIW, I think that probably the best course of action is to go ahead
> and install POSIX-compliant error checking in the existing functions
> too. POSIX:2008 is quite clear about this:
>
> : An application wishing to check for error situations should set errno to
> : zero and call feclearexcept(FE_ALL_EXCEPT) before calling these
> : functions. On return, if errno is non-zero or fetestexcept(FE_INVALID |
> : FE_DIVBYZERO | FE_OVERFLOW | FE_UNDERFLOW) is non-zero, an error has
> : occurred.
>

Looking at this again, I think it makes sense to make the Inf/NaN
handling of these functions platform-independent. POSIX.1-2008 is
pretty explicit about how they ought to behave, which is different
from the current behaviour on either Linux or Windows:

sin(Inf):
POSIX: domain error
Linux: NaN
Windows: ERROR: input is out of range

asin(Inf):
POSIX: domain error
Linux: ERROR: input is out of range
Windows: ERROR: input is out of range

sin(NaN):
POSIX: NaN
Linux: NaN
Windows: ERROR: input is out of range

asin(NaN):
POSIX: NaN
Linux: NaN
Windows: ERROR: input is out of range

I tried using feclearexcept + fetestexcept as recommended by
POSIX.1-2008, and that does indeed make the above examples compliant
on my linux box. Unfortunately it introduces new errors -- asin starts
generating FE_UNDERFLOW errors for numbers that are really not that
small, such as asin(1e-9), although the function still returns the
correct answer. A bigger problem though is that these are C99
functions and so won't necessarily be available on all supported
platforms.

So I think that a simpler answer is to just to add explicit tests for
these inputs and avoid relying on errno, at least for the inverse
functions, which have pretty clear constraints on their allowed
inputs. For the forward functions, I'm not sure if there are some
platforms on which large but finite inputs might generate errors, so I
think it's safest to keep the existing errno checks as well just in
case.

Attached are patches for this and the new functions in degrees, now
with POSIX compatible Inf/NaN handling.

Regards,
Dean

On Mon, Jan 18, 2016 at 5:01 AM, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
> On 30 November 2015 at 14:11, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> FWIW, I think that probably the best course of action is to go ahead
>> and install POSIX-compliant error checking in the existing functions
>> too. POSIX:2008 is quite clear about this:
>>
>> : An application wishing to check for error situations should set errno to
>> : zero and call feclearexcept(FE_ALL_EXCEPT) before calling these
>> : functions. On return, if errno is non-zero or fetestexcept(FE_INVALID |
>> : FE_DIVBYZERO | FE_OVERFLOW | FE_UNDERFLOW) is non-zero, an error has
>> : occurred.
>>
>
> Looking at this again, I think it makes sense to make the Inf/NaN
> handling of these functions platform-independent. POSIX.1-2008 is
> pretty explicit about how they ought to behave, which is different
> from the current behaviour on either Linux or Windows:
>
> sin(Inf):
> POSIX: domain error
> Linux: NaN
> Windows: ERROR: input is out of range

OSX: NaN

> asin(Inf):
> POSIX: domain error
> Linux: ERROR: input is out of range
> Windows: ERROR: input is out of range

OSX: NaN

> sin(NaN):
> POSIX: NaN
> Linux: NaN
> Windows: ERROR: input is out of range

OSX: NaN

> asin(NaN):
> POSIX: NaN
> Linux: NaN
> Windows: ERROR: input is out of range

OSX: NaN.

> I tried using feclearexcept + fetestexcept as recommended by
> POSIX.1-2008, and that does indeed make the above examples compliant
> on my linux box. Unfortunately it introduces new errors -- asin starts
> generating FE_UNDERFLOW errors for numbers that are really not that
> small, such as asin(1e-9), although the function still returns the
> correct answer. A bigger problem though is that these are C99
> functions and so won't necessarily be available on all supported
> platforms.
>
> So I think that a simpler answer is to just to add explicit tests for
> these inputs and avoid relying on errno, at least for the inverse
> functions, which have pretty clear constraints on their allowed
> inputs. For the forward functions, I'm not sure if there are some
> platforms on which large but finite inputs might generate errors, so I
> think it's safest to keep the existing errno checks as well just in
> case.

Thanks for this investigation. That's really inconsistent...

> Attached are patches for this and the new functions in degrees, now
> with POSIX compatible Inf/NaN handling.

The first patch looks fine to me, a nitpick:
+ /* Be sure to throw an error if the input is infinite --- see dcos */
s/dcos/docs

For patch 2, another nitpick :)
+ return ( sin(x * (M_PI / 180.0)) / sin(30.0 * (M_PI / 180.0)) ) / 2.0;
parenthesis format looks incorrect, too many spaces at the border.

Except those things the second patch looks good to me as well. Let's
have a committer look at it.
--
Michael

On 19 January 2016 at 06:32, Michael Paquier <michael(dot)paquier(at)gmail(dot)com> wrote:
> The first patch looks fine to me, a nitpick:
> + /* Be sure to throw an error if the input is infinite --- see dcos */
> s/dcos/docs
>

No, I meant dcos the function there. I would normally write that as
dcos() to indicate that it is a function, but I thought the convention
here was to omit the parentheses after function names. Looking again,
I see several examples of function names with parentheses in comments,
so they could be added here, or the comment could be written a
different way. I'm happy to leave that to the committer's discretion.

> For patch 2, another nitpick :)
> + return ( sin(x * (M_PI / 180.0)) / sin(30.0 * (M_PI / 180.0)) ) / 2.0;
> parenthesis format looks incorrect, too many spaces at the border.
>
> Except those things the second patch looks good to me as well. Let's
> have a committer look at it.

OK. Thanks for looking at this.

Regards,
Dean

Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> writes:
> I tried using feclearexcept + fetestexcept as recommended by
> POSIX.1-2008, and that does indeed make the above examples compliant
> on my linux box. Unfortunately it introduces new errors -- asin starts
> generating FE_UNDERFLOW errors for numbers that are really not that
> small, such as asin(1e-9), although the function still returns the
> correct answer. A bigger problem though is that these are C99
> functions and so won't necessarily be available on all supported
> platforms.

The latter issue could be dealt with via configure tests, but I agree that
if we get new error conditions we weren't exactly looking for, it would
not be a net improvement. I think ensuring that the Inf and NaN cases are
platform-independent is already a good step forward, so we can stop there
for now.

> Attached are patches for this and the new functions in degrees, now
> with POSIX compatible Inf/NaN handling.

Pushed with minor, mostly cosmetic fixes. I did renumber the function
OIDs to be closer to the original trig functions' numbers, using some
OIDs that were freed up by the recent index AM API change.

regards, tom lane

I wrote:
> Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> writes:
>> Attached are patches for this and the new functions in degrees, now
>> with POSIX compatible Inf/NaN handling.

> Pushed with minor, mostly cosmetic fixes.

So the early returns from the buildfarm aren't very good:

* tern/sungazer isn't getting exactly 0.5 from sind(30).

* narwhal isn't getting exactly 1 from tand(45) or cotd(45).
It also is producing "0" not "-0" from tand(180) and cotd(270).

* gaur likewise is getting "0" not "-0" from tand(180) and cotd(270).

The tern/sungazer result implies that this:

return (sin(x * (M_PI / 180.0)) / sin(30.0 * (M_PI / 180.0))) / 2.0;

is not producing exactly 0.5, which means that the two sin() calls aren't
producing identical results, which I suspect is best explained by the
theory that the compiler is rearranging 30.0 * (M_PI / 180.0) into
(30.0 * M_PI) / 180.0, and getting a slightly different number that way.

I think we could fix that by replacing (M_PI / 180.0) by a hard-wired
constant (computed to say 20 digits or so). I'd be inclined to do that
throughout the file for consistency; however, in principle such a change
might affect existing results from the radians() and degrees() functions.

The tand(45) problem doesn't seem especially surprising, because we're
really not making any effort to ensure that that result is exact.
A brute-force way to fix it would be to divide
(sind_q1(arg1) / cosd_q1(arg1)) by (sind_q1(45.0) / cosd_q1(45.0))
but that seems rather expensive, and possibly subject to the compiler
deciding to reorder the arithmetic operations. I wonder if there's a
smarter way.

As for the minus-zero issues, I doubt that (a) there is a basis for
claiming that minus-zero is more correct than plain zero, or (b) the user
community for this feature really wants minus-zero results anyhow.
I propose getting rid of minus-zero outputs from tand and cotd by dint
of code like

if (result == 0.0)
result = 0.0;

Thoughts?

regards, tom lane

I wrote:
> So the early returns from the buildfarm aren't very good:
> * tern/sungazer isn't getting exactly 0.5 from sind(30).

> The tern/sungazer result implies that this:

> return (sin(x * (M_PI / 180.0)) / sin(30.0 * (M_PI / 180.0))) / 2.0;

> is not producing exactly 0.5, which means that the two sin() calls aren't
> producing identical results, which I suspect is best explained by the
> theory that the compiler is rearranging 30.0 * (M_PI / 180.0) into
> (30.0 * M_PI) / 180.0, and getting a slightly different number that way.

> I think we could fix that by replacing (M_PI / 180.0) by a hard-wired
> constant (computed to say 20 digits or so).

So I pushed that, and tern/sungazer are still failing. Noah, could you
trace through that and see exactly where it's going off the rails?

regards, tom lane

On 1/23/16 12:08 PM, Tom Lane wrote:
> So I pushed that, and tern/sungazer are still failing. Noah, could you
> trace through that and see exactly where it's going off the rails?

I'm still getting a failure in float8 on OS X after commit 73193d8:

@@ -490,9 +490,9 @@
x | asind | acosd | atand
------+-------+-------+-------
-1 | -90 | 180 | -45
- -0.5 | -30 | 120 |
+ -0.5 | | 120 |
0 | 0 | 90 | 0
- 0.5 | 30 | 60 |
+ 0.5 | | |
1 | 90 | 0 | 45

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> On 1/23/16 12:08 PM, Tom Lane wrote:
>> So I pushed that, and tern/sungazer are still failing. Noah, could you
>> trace through that and see exactly where it's going off the rails?

> I'm still getting a failure in float8 on OS X after commit 73193d8:

Weird, because my OS X critters are all happy. Which OS X and compiler
version, exactly? Any special compile flags?

regards, tom lane

On Sat, Jan 23, 2016 at 12:08:40PM -0500, Tom Lane wrote:
> I wrote:
> > So the early returns from the buildfarm aren't very good:
> > * tern/sungazer isn't getting exactly 0.5 from sind(30).
>
> > The tern/sungazer result implies that this:
>
> > return (sin(x * (M_PI / 180.0)) / sin(30.0 * (M_PI / 180.0))) / 2.0;
>
> > is not producing exactly 0.5, which means that the two sin() calls aren't
> > producing identical results, which I suspect is best explained by the
> > theory that the compiler is rearranging 30.0 * (M_PI / 180.0) into
> > (30.0 * M_PI) / 180.0, and getting a slightly different number that way.
>
> > I think we could fix that by replacing (M_PI / 180.0) by a hard-wired
> > constant (computed to say 20 digits or so).
>
> So I pushed that, and tern/sungazer are still failing. Noah, could you
> trace through that and see exactly where it's going off the rails?

The second sin() is a constant, so gcc computes it immediately but sends the
first sin() to libm. The libm sin() is slightly more accurate. In %a
notation, AIX libm computes sin(30.0 * RADIANS_PER_DEGREE) as 0x1p-1 while gcc
computes it as 0x1.fffffffffffffp-2, a difference of one ULP. (Both "30.0 *
RADIANS_PER_DEGREE" and "30.0 * (M_PI / 180.0)" match the runtime computation
of 0x1.0c152382d7365p-1.)

To reliably produce exact answers, this code must delay all trigonometric
calculations to runtime. On sungazer, the float8 test happens to pass if I
rebuild float.c with -fno-builtin-sin; that leaves calls like acos(0.5) and
cos(60.0 * RADIANS_PER_DEGREE) unprotected, though.

Noah Misch <noah(at)leadboat(dot)com> writes:
> On Sat, Jan 23, 2016 at 12:08:40PM -0500, Tom Lane wrote:
>> So I pushed that, and tern/sungazer are still failing. Noah, could you
>> trace through that and see exactly where it's going off the rails?

> The second sin() is a constant, so gcc computes it immediately but sends the
> first sin() to libm. The libm sin() is slightly more accurate.

Ugh. "compile-time simplification uses different version of sin()" was
one of the theories I had in mind, but I was hoping that wasn't it
because it'd be the hardest to work around reliably. Still, I think it's
doable by caching the results of the should-be-constant subexpressions.
Will get on it.

regards, tom lane

On 1/23/16 3:05 PM, Tom Lane wrote:
> Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
>> On 1/23/16 12:08 PM, Tom Lane wrote:
>>> So I pushed that, and tern/sungazer are still failing. Noah, could you
>>> trace through that and see exactly where it's going off the rails?
>
>> I'm still getting a failure in float8 on OS X after commit 73193d8:
>
> Weird, because my OS X critters are all happy. Which OS X and compiler
> version, exactly? Any special compile flags?

I'm using gcc 4.8. It passes with the system clang. So the explanation
is probably along the lines of what Noah has described.

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> On 1/23/16 3:05 PM, Tom Lane wrote:
>> Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
>>> I'm still getting a failure in float8 on OS X after commit 73193d8:

>> Weird, because my OS X critters are all happy. Which OS X and compiler
>> version, exactly? Any special compile flags?

> I'm using gcc 4.8. It passes with the system clang. So the explanation
> is probably along the lines of what Noah has described.

Ah. Please see if what I just pushed fixes it.

regards, tom lane

Noah Misch <noah(at)leadboat(dot)com> writes:
> To reliably produce exact answers, this code must delay all trigonometric
> calculations to runtime. On sungazer, the float8 test happens to pass if I
> rebuild float.c with -fno-builtin-sin; that leaves calls like acos(0.5) and
> cos(60.0 * RADIANS_PER_DEGREE) unprotected, though.

Either I missed something or there's another issue, because tern/sungazer
are *still* failing. This is getting annoying :-(

regards, tom lane

On Sat, Jan 23, 2016 at 05:04:56PM -0500, Tom Lane wrote:
> Noah Misch <noah(at)leadboat(dot)com> writes:
> > To reliably produce exact answers, this code must delay all trigonometric
> > calculations to runtime. On sungazer, the float8 test happens to pass if I
> > rebuild float.c with -fno-builtin-sin; that leaves calls like acos(0.5) and
> > cos(60.0 * RADIANS_PER_DEGREE) unprotected, though.
>
> Either I missed something or there's another issue, because tern/sungazer
> are *still* failing. This is getting annoying :-(

sungazer's "make check" passes if I change init_degree_constants() to be
non-static. Duping gcc isn't so easy these days.

Noah Misch <noah(at)leadboat(dot)com> writes:
> On Sat, Jan 23, 2016 at 05:04:56PM -0500, Tom Lane wrote:
>> Either I missed something or there's another issue, because tern/sungazer
>> are *still* failing. This is getting annoying :-(

> sungazer's "make check" passes if I change init_degree_constants() to be
> non-static. Duping gcc isn't so easy these days.

Ugh. Well, at least we don't have to move it to another file, which was
going to be my next larger size of hammer.

Thanks for doing the legwork on this!

regards, tom lane

On 23 January 2016 at 23:04, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Noah Misch <noah(at)leadboat(dot)com> writes:
>> On Sat, Jan 23, 2016 at 05:04:56PM -0500, Tom Lane wrote:
>>> Either I missed something or there's another issue, because tern/sungazer
>>> are *still* failing. This is getting annoying :-(
>
>> sungazer's "make check" passes if I change init_degree_constants() to be
>> non-static. Duping gcc isn't so easy these days.
>
> Ugh. Well, at least we don't have to move it to another file, which was
> going to be my next larger size of hammer.
>
> Thanks for doing the legwork on this!
>

Hi, I'm just now catching up on my email after being out of town and
not reading it. Thanks for looking at this and sorting out those
issues, and thank you also Noah and Peter for your investigative work.

If I understand correctly there were 2 separate issues at play here:

1). On some platforms the compiler evaluates expressions like
sin(constant) and comes up with a slightly different result than a
runtime evaluation of the expression. The compiler-evaluated result is
presumably a 64-bit IEEE float, but at runtime it may be using
extended precision for intermediate results. That may well have been
the sole contributing factor to the fact that sind(30) wasn't exactly
0.5.

2). The compiler also sometimes rearranges expressions in ways that
don't give the same result as evaluating in the order suggested by the
parentheses. I think this actually explains the failure to get exactly
1 for tand(45). For x=45, this was being computed as

cosd_0_to_60(90 - x) / cosd_0_to_60(x)

so my guess is that it was inlining cosd_0_to_60(90 - x) and
rearranging it to produce something different from cosd_0_to_60(x) for
x=45.

Looking at the new code, it's annoying how much effort was needed to
prevent the compiler messing it up. I ought to have realised that the
optimiser would be awkward for this kind of thing.

I wonder if the same could have been achieved by disabling
optimisation and inlining in those low-level functions, and also
wrapping sin(x * RADIANS_PER_DEGREE) in a similar non-inlinable,
non-optimised function to force it to be executed at runtime when
passed a constant. That would probably have made them significantly
slower though, whereas the new code benefits from various pre-computed
expressions.

Thanks again for fixing this.

Regards,
Dean

Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> writes:
> If I understand correctly there were 2 separate issues at play here:

> 1). On some platforms the compiler evaluates expressions like
> sin(constant) and comes up with a slightly different result than a
> runtime evaluation of the expression. The compiler-evaluated result
> is presumably a 64-bit IEEE float, but at runtime it may be using
> extended precision for intermediate results.

If I've not lost track of the bidding, both of the cases where we saw
that involved gcc on a platform where it's not the native (vendor
supplied) compiler. So my guess is that gcc was using a non-native
libm to do the pre-evaluation of sin(). It does not seem likely that
the gcc boys would intentionally do pre-evaluation differently from
run-time evaluation, but they could get burnt by what would arguably
be a build error in that particular copy of gcc. Cross-compilation
would be another way to hit the same type of hazard.

> That may well have been the sole contributing factor to the fact that
> sind(30) wasn't exactly 0.5.

Yeah. I am not sure whether the RADIANS_PER_DEGREE change fixed anything
or not, though I am not tempted to undo it; it may save us on some other
platform not currently represented in the buildfarm.

> 2). The compiler also sometimes rearranges expressions in ways that
> don't give the same result as evaluating in the order suggested by the
> parentheses. I think this actually explains the failure to get exactly
> 1 for tand(45). For x=45, this was being computed as
> cosd_0_to_60(90 - x) / cosd_0_to_60(x)
> so my guess is that it was inlining cosd_0_to_60(90 - x) and
> rearranging it to produce something different from cosd_0_to_60(x) for
> x=45.

Oh, interesting point. The inlining would have produced a subexpression
like

cos((90 - x) * RADIANS_PER_DEGREE)

For x=45, the result of 90-x would have been exact, so it's not obvious
where any change in results would have crept in --- but if the compiler
then tried to simplify to

cos((90 * RADIANS_PER_DEGREE) - (x * RADIANS_PER_DEGREE))

that could definitely change the roundoff behavior. OTOH, it's not
very clear why gcc would have done that; it saves no operations.
It'd be interesting to look at the produced assembly code on narwhal.

> I wonder if the same could have been achieved by disabling
> optimisation and inlining in those low-level functions, and also
> wrapping sin(x * RADIANS_PER_DEGREE) in a similar non-inlinable,
> non-optimised function to force it to be executed at runtime when
> passed a constant.

I considered that; in particular -ffloat-store would have helped with the
wider-intermediate-results problem, and indeed we might still be forced
into using that. I would rather not fall back to adding more
compiler-specific flags though. If we go that way we'll likely need a
custom fix for every new compiler we try to use.

Meanwhile, just when you thought it was safe to go back in the water,
cockatiel is still failing. It has the cos(60) != 0.5 problem, which
IIRC was exhibited by no other critter. Looking at the code,

cosd_0_to_60(double x)
{
return 1.0 - ((1.0 - cos(x * RADIANS_PER_DEGREE)) / one_minus_cos_60) / 2.0;
}

what seems likely is that the "1 - cos()" subtraction is being done in
a wider-than-double float register, which i686 does have, and producing
a different result than what was stored in one_minus_cos_60.

Perhaps we can fix this by rewriting as

float8 numerator = 1.0 - cos(x * RADIANS_PER_DEGREE);
return 1.0 - (numerator / one_minus_cos_60) / 2.0;

cockatiel's compiler does recognize -fexcess-precision=standard, and
my understanding of that is that the result put into "numerator" will
be rounded to double width, so that it should then match
"one_minus_cos_60".

Another idea would be to change the cache variable to just "cos_60" and
write "(1.0 - cos_60)" in the denominator --- but then we'd just be hoping
that the compiler does both subtractions the same way, which doesn't seem
necessarily guaranteed. Worse, I believe the 8087 has an FCOS instruction
which might deliver a wider-than-double result, so that maybe the problem
is not so much with the subtraction as with when rounding of the cos()
result happens. The code I show above seems more likely to match the
way one_minus_cos_60 is computed.

I'll go try it, though I guess we won't see results till tomorrow.

regards, tom lane

On 1/23/16 4:18 PM, Tom Lane wrote:
> Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
>> On 1/23/16 3:05 PM, Tom Lane wrote:
>>> Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
>>>> I'm still getting a failure in float8 on OS X after commit 73193d8:
>
>>> Weird, because my OS X critters are all happy. Which OS X and compiler
>>> version, exactly? Any special compile flags?
>
>> I'm using gcc 4.8. It passes with the system clang. So the explanation
>> is probably along the lines of what Noah has described.
>
> Ah. Please see if what I just pushed fixes it.

Works now. Thanks.

On Mon, Jan 25, 2016 at 2:34 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Perhaps we can fix this by rewriting as
>
> float8 numerator = 1.0 - cos(x * RADIANS_PER_DEGREE);
> return 1.0 - (numerator / one_minus_cos_60) / 2.0;
>
> cockatiel's compiler does recognize -fexcess-precision=standard, and
> my understanding of that is that the result put into "numerator" will
> be rounded to double width, so that it should then match
> "one_minus_cos_60".
>
> Another idea would be to change the cache variable to just "cos_60" and
> write "(1.0 - cos_60)" in the denominator --- but then we'd just be hoping
> that the compiler does both subtractions the same way, which doesn't seem
> necessarily guaranteed. Worse, I believe the 8087 has an FCOS instruction
> which might deliver a wider-than-double result, so that maybe the problem
> is not so much with the subtraction as with when rounding of the cos()
> result happens. The code I show above seems more likely to match the
> way one_minus_cos_60 is computed.

(Sorry for showing up after the storm)
The fix is working correctly, using gcc's i686-pc-cygwin on cygwin32
the regression does not show up anymore
after 0034757.
--
Michael

These changes from 65abaab547a5758b0d6d92df4af1663bb47d545f

- result = sign * cosd_q1(arg1) / sind_q1(arg1);
+ result = sign * ((cosd_q1(arg1) / sind_q1(arg1)) / cot_45);

and

- result = sign * sind_q1(arg1) / cosd_q1(arg1);
+ result = sign * ((sind_q1(arg1) / cosd_q1(arg1)) / tan_45);

both introduce division by zero, don't they?

On Sun, Jan 31, 2016 at 9:01 PM, Piotr Stefaniak
<postgres(at)piotr-stefaniak(dot)me> wrote:
> - result = sign * cosd_q1(arg1) / sind_q1(arg1);
> + result = sign * ((cosd_q1(arg1) / sind_q1(arg1)) / cot_45);
>
> and
>
> - result = sign * sind_q1(arg1) / cosd_q1(arg1);
> + result = sign * ((sind_q1(arg1) / cosd_q1(arg1)) / tan_45);
>
> both introduce division by zero, don't they?

Per IEEE 754, division by 0 for a double results in Nan or +/-Inf, so
that's actually correct.
--
Michael

On 01/31/2016 01:23 PM, Michael Paquier wrote:
> Per IEEE 754, division by 0 for a double results in Nan or +/-Inf, so
> that's actually correct.

I didn't know that. I guess that in practice that is OK and the case is
closed.

Interestingly to me, that assumption appears to rely on the C
implementation complying to IEC 60559, in which case C99 lets the
implementation signal that by defining the __STDC_IEC_559__ macro. C89
doesn't seem to mention any of this.

Piotr Stefaniak <postgres(at)piotr-stefaniak(dot)me> writes:
> These changes from 65abaab547a5758b0d6d92df4af1663bb47d545f
> - result = sign * cosd_q1(arg1) / sind_q1(arg1);
> + result = sign * ((cosd_q1(arg1) / sind_q1(arg1)) / cot_45);

> and

> - result = sign * sind_q1(arg1) / cosd_q1(arg1);
> + result = sign * ((sind_q1(arg1) / cosd_q1(arg1)) / tan_45);

> both introduce division by zero, don't they?

Huh? cot_45 and tan_45 are fixed values that should be very close to 1.
Or were you complaining that the potential div by 0 existed beforehand?

It's possible that we should check for sind_q1(arg1) or cosd_q1(arg1)
being zero before we try the divide, and substitute get_float8_infinity()
instead. But the regression tests exercise this case, and none of the
buildfarm members complained, so I'm a bit disinclined to add code for
that purpose. If anyone does report regression test failure here, we can
revisit the issue then.

regards, tom lane