Re: Proposal: Trigonometric functions in degrees

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