Opened 7 weeks ago

Closed 6 weeks ago

#1545 closed defect (fixed)

pow() doesn't work for negative values

Reported by: dmisev Owned by: bbell
Priority: major Milestone: 9.5
Component: qlparser Version: development
Keywords: Cc: vmerticariu, pbaumann
Complexity: Medium

Description (last modified by dmisev)

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 Changed 7 weeks ago by dmisev

  • Description modified (diff)

comment:2 Changed 7 weeks ago by bbell

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 Changed 7 weeks ago by dmisev

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 Changed 7 weeks ago by dmisev

python's pow() gives a Domain error though

comment:5 Changed 7 weeks ago by bbell

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

comment:6 Changed 7 weeks ago by dmisev

  • Cc vmerticariu 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 Changed 7 weeks ago by dmisev

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 Changed 7 weeks ago by bbell

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 Changed 7 weeks ago by bbell

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 Changed 7 weeks ago by dmisev

  • Cc pbaumann 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 Changed 7 weeks ago by pbaumann

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 Changed 7 weeks ago by dmisev

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 Changed 7 weeks ago by pbaumann

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 follow-up: Changed 7 weeks ago by dmisev

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.

comment:15 in reply to: ↑ 14 Changed 7 weeks ago by dmisev

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 Changed 7 weeks ago by dmisev

  • Milestone changed from 9.4 to 9.5

comment:17 Changed 6 weeks ago by bbell

  • Resolution set to fixed
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.