Opened 7 years ago

Closed 7 years ago

#1545 closed defect (fixed)

pow() doesn't work for negative values

Reported by: Dimitar Misev Owned by: bbell
Priority: major Milestone: 9.5
Component: qlparser Version: development
Keywords: Cc: Vlad Merticariu, Peter Baumann
Complexity: Medium

Description (last modified by Dimitar Misev)

As reported on the mailing list:

in expression > pow(x, 0.756), if x evaluates to 0 or greater, everything works, but if evaluates to negative value, an error 400 is thrown. The rasserver log displays only this:
Error number: 510 Token: POW Line: 1 Column: 15

A quick test to reproduce:

$ directql -q 'select csv(pow(<[0:0] -5f>, 0.765))' --out string -d /d/rasdata/RASBASE 
directql: rasdaman query tool v1.0, rasdaman v9.4.0-beta2.
opening database /d/rasdata/RASBASE at localhost:7001... [INFO] - Connected successfully to '/d/rasdata/RASBASE'
 [INFO] - initialized blob file storage handler with root data directory /d/rasdata/TILES/
 ok
Executing retrieval query... [INFO] - Request: 'select csv(pow(<[0:0] -5f>, 0.765))'...
 [INFO] - parsing...
 [INFO] - checking semantics...
 [INFO] - evaluating...
[FATAL] - QtUnaryInduce::computeUnaryMDDOp caught errno error (510) in unaryinduce
Getting result...ok.
directql done.

Change History (17)

comment:1 by Dimitar Misev, 7 years ago

Description: modified (diff)

comment:2 by bbell, 7 years ago

So, your query is to take a fractional exponent of a negative number?

Are you sure that the real numbers are your domain of interest here?

comment:3 by Dimitar Misev, 7 years ago

Did you see the example I provided:

select csv(pow(<[0:0] -5f>, 0.765))

My calculator has no issue calculating -5.00.765

comment:4 by Dimitar Misev, 7 years ago

python's pow() gives a Domain error though

comment:5 by bbell, 7 years ago

So basically we should return nan for now, and not worry about complex numbers for the time being?

comment:6 by Dimitar Misev, 7 years ago

Cc: Vlad Merticariu added

I'm not sure. Could you check the documentation of std::pow and see whether this is really an error case (because errno is set after all), or -nan is an acceptable result that makes sense?

comment:7 by Dimitar Misev, 7 years ago

python throws a Domain error, postgres throws ERROR: a negative number raised to a non-integer power yields a complex result.

I think rasdaman is doing the right thing here and throws an appropriate error. The only problematic issue is that the rasserver log contains a not very good error:
Error number: 510 Token: pow Line: 1 Column: 8

rasql shows a much better error: rasdaman error 510: Execution error 510 in line 1, column 8, near token pow: The argument is outside the function domain.

but a petascope user doesn't see this.

comment:8 by bbell, 7 years ago

regarding std::pow, it returns -nan and throws an error (see errno.h and errno-base.h); however, I do not think that such an error should be considered fatal. My position on what the best approach is in this case mirrors Clements' perspective more closely.

I think that it is a good idea to put more-useful information in the rasserver log; however, this ostream operation is called by our general error parser for rasql, and making changes in the output there will change how every error resulting from rasql appears in the rasserver log (unless we want to create a very special situation just for errorno 510). There will be other times when this information is useful to the user, so I am in support of the general approach.

comment:9 by bbell, 7 years ago

http://en.cppreference.com/w/c/numeric/math/pow
nice reference with examples that show how the error handling is performed. our implementation in rasdaman is quite similar

comment:10 by Dimitar Misev, 7 years ago

Cc: Peter Baumann added

Ok so let's eliminate this particular error check in rasdaman. In particular, in case of invalid domain rasdaman will not throw an exception, but rather return what the C++ pow() returns: -nan.

Anyone objecting?

comment:11 by Peter Baumann, 7 years ago

not against it, but let's carefully evaluate: existing applications out there might already rely on a particular behaviour = not be prepared to operate with a change to non-error, nan results.
Further: this discussion is only on floats - we would deliver appropriate complex results for complex input, right? (cf Brennan's concern)

comment:12 by Dimitar Misev, 7 years ago

I don't think pow() on complex is implemented.

What can we do about this?

existing applications out there might already rely on a particular behaviour = not be prepared to operate with a change to non-error, nan results

Should we ask on the mailing list if anyone has an objection? Nobody seemed to be against, and at least two were in favor.

comment:13 by Peter Baumann, 7 years ago

yes, a good move: ask the list. Likewise, I do not expect objections, but let us ask.
re pow() on complex: we should file an extra ticket. Will become important in the near future IMHO.

comment:14 by Dimitar Misev, 7 years ago

I asked; but if we do this we should consider the other similar cases where we throw errors as well:

  • log(0) = -inf
  • acos(10)/asin(10) = nan
  • etc.

I think we should similarly convert them to warnings.

in reply to:  14 comment:15 by Dimitar Misev, 7 years ago

Replying to dmisev:

I think we should similarly convert them to warnings.

Here's a full list (510 = domain error, 511 = overflow error):

catalogmgr$ ag throw autogen_ops.cc -B6 | awk '/throw/ || /convRes/'
57-        convRes = fabs(convOp);
63:            throw 510;
67:            throw 511;
92-        convRes = sqrt(convOp);
98:            throw 510;
102:            throw 511;
127-        convRes = pow(convOp, exponent);
133:            throw 510;
137:            throw 511;
167-        convRes = exp(convOp);
173:            throw 510;
177:            throw 511;
202-        convRes = log10(convOp);
208:            throw 510;
212:            throw 511;
237-        convRes = log(convOp);
243:            throw 510;
247:            throw 511;
272-        convRes = sin(convOp);
278:            throw 510;
282:            throw 511;
307-        convRes = cos(convOp);
313:            throw 510;
317:            throw 511;
342-        convRes = tan(convOp);
348:            throw 510;
352:            throw 511;
377-        convRes = sin(convOp);
383:            throw 510;
387:            throw 511;
412-        convRes = cos(convOp);
418:            throw 510;
422:            throw 511;
447-        convRes = tan(convOp);
453:            throw 510;
457:            throw 511;
482-        convRes = asin(convOp);
488:            throw 510;
492:            throw 511;
517-        convRes = acos(convOp);
523:            throw 510;
527:            throw 511;
552-        convRes = atan(convOp);
558:            throw 510;
562:            throw 511;

comment:16 by Dimitar Misev, 7 years ago

Milestone: 9.49.5

comment:17 by bbell, 7 years ago

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.