Opened 18 months ago

Last modified 16 months ago

#24428 needs_review defect

Numerical evaluation should return a complex number if applicable

Reported by: jdemeyer Owned by:
Priority: critical Milestone: sage-8.2
Component: symbolics Keywords:
Cc: rws, slelievre Merged in:
Authors: Ralf Stephan Reviewers:
Report Upstream: N/A Work issues:
Branch: u/rws/24428 (Commits) Commit: aa041a972d4b09a1e8b771b370e57f9f9ac19f7b
Dependencies: #24832, pynac-0.7.17 Stopgaps:

Description (last modified by rws)

This looks wrong:

sage: arccosh(0.9)
NaN

Especially given all the following:

sage: arccosh(RDF(0.9))
0.45102681179626236*I
sage: arccosh(x).subs(x=0.9)
0.451026811796262*I
sage: sqrt(-2.0)
1.41421356237310*I

A complex number is more useful than a NaN so we shouldn't return NaN in the first example.

The Function code first calls x.arccosh() which returns the NaN. The reason for only the RDF case working is that RDF does not have a arccosh member function so the computation is delegated to Pynac where the complex value is returned.

Change History (27)

comment:1 Changed 18 months ago by jdemeyer

  • Description modified (diff)

comment:2 Changed 18 months ago by rws

I'm not decided on which result is correct. But see also #15344.

comment:3 Changed 18 months ago by rws

so the first should probably return a complex number too

This would mean either 1. changing general evaluation of f(arg) to not try arg.f() first or, 2. changing the interface to mpfr_acosh() (which is responsible for the NaN), i.e. RR.arccosh(), and some others too.

I'm in favor of the latter.

comment:4 Changed 18 months ago by rws

Note that RBF(0.9).arccosh() returns nan as well.

comment:5 Changed 18 months ago by mantepse

Does an expression in SR (like acosh) come with a notion of domain and codomain?

comment:6 Changed 18 months ago by rws

Symbolic function expressions have internal restrictions as to their arguments but there is no information associated regarding domains. The function code in sage/functions and in Pynac raises stock Python exceptions and runtime errors if nonsensical arguments are encountered, but just yesterday I wished I could catch them specifically, e.g. by inheriting from domain error---it would enable much better random testing.

comment:7 Changed 18 months ago by mantepse

It might be important to note that this is a regression:

sage: arccosh(0.9)
NaN
sage: arccosh(x).subs(x=0.9)
NaN
sage: version()
'SageMath version 8.1.beta5, Release Date: 2017-09-11'

comment:8 Changed 18 months ago by mantepse

in fact, the change must have been introduced in 8.2.beta1, because in 8.2.beta0 it still gives the expected result.

comment:9 Changed 18 months ago by rws

That is no surprise as I changed FP evaluation in the commit https://github.com/pynac/pynac/commit/d0f66f94ab4564a9a43aaf5907f7ac2a90047890

It might not be a bug. Still, the necessity of being consistent demands some fix somewhere.

comment:10 Changed 18 months ago by rws

For example

sage: arccos(1.1)
NaN
sage: arccos(x).subs(x=1.1)
NaN

(EDITED)

So one of arccos/arccosh should be changed.

Last edited 18 months ago by rws (previous) (diff)

comment:11 Changed 18 months ago by mantepse

Changes in symbolics code are quite likely to produce doctest differences in the fricas interface, so it might make sense to make sure that these doctests are run.

In the case at hand, I guess it would be very important to specify domain and codomain of expressions which can be evaluated numerically, otherwise it will never be clear whether something is a bug or a feature.

Besides, I think that arccosh is terrible language :-)

comment:12 follow-up: Changed 16 months ago by slelievre

  • Cc slelievre added
  • Description modified (diff)

Adding an example:

sage: arccosh(RDF(0.9))
0.45102681179626236*I

comment:13 Changed 16 months ago by jdemeyer

  • Description modified (diff)
  • Summary changed from Substitution should be the same as numerical evaluation to Numerical evaluation should return a complex number if applicable

comment:14 follow-up: Changed 16 months ago by vdelecroix

I don't care what you do with the function arccosh but all of

sage: RR(0.9).arccosh()
NaN
sage: RBF(0.9).arccosh()
nan
sage: RIF(0.9).arccosh()
[.. NaN ..]

must not change.

comment:15 in reply to: ↑ 14 Changed 16 months ago by rws

With pynac-0.7.17:

sage: acos(SR(1.1))
0.443568254385115*I
sage: acosh(SR(0.9))
0.451026811796262*I
sage: acos(x).subs(x=1.1)
0.443568254385115*I
sage: acosh(x).subs(x=0.9)
0.451026811796262*I

Replying to vdelecroix:

I don't care what you do with the function arccosh but all of

sage: RR(0.9).arccosh()
NaN
sage: RBF(0.9).arccosh()
nan
sage: RIF(0.9).arccosh()
[.. NaN ..]

must not change.

That is however the reason for

sage: acos(1.1)
NaN

and all the others because here (1.1).acos() is called first.

comment:16 in reply to: ↑ 12 ; follow-up: Changed 16 months ago by rws

Replying to slelievre:

Adding an example:

sage: arccosh(RDF(0.9))
0.45102681179626236*I

Note that this only works because RDF has no arccosh member function, so the computation is delegated to Pynac.

comment:17 in reply to: ↑ 16 ; follow-up: Changed 16 months ago by vdelecroix

Replying to rws:

Replying to slelievre:

Adding an example:

sage: arccosh(RDF(0.9))
0.45102681179626236*I

Note that this only works because RDF has no arccosh member function, so the computation is delegated to Pynac.

In this situation you could just fallback to acosh

sage: RDF(0.9).acosh()
NaN

comment:18 in reply to: ↑ 17 Changed 16 months ago by rws

Replying to vdelecroix:

sage: arccosh(RDF(0.9))
0.45102681179626236*I

Note that this only works because RDF has no arccosh member function, so the computation is delegated to Pynac.

In this situation you could just fallback to acosh

sage: RDF(0.9).acosh()
NaN

Instead of special casing I'd rather change the name of the inverse hyperbolic functions away from all the wrong arcs (arcus) to the as and have ar (area) aliases which would be the correct prefix.

Moreover, if I get a review on the change to always delegate to _eval_() (EDITED) if NaN is returned then I'd change that too.

Last edited 16 months ago by rws (previous) (diff)

comment:19 Changed 16 months ago by rws

  • Description modified (diff)

comment:20 follow-ups: Changed 16 months ago by rws

So why is this not a bug?!

sage: RR(-2).sqrt()
1.41421356237310*I

comment:22 in reply to: ↑ 20 Changed 16 months ago by vdelecroix

Replying to rws:

Replying to slelievre:

Adding an example:

sage: arccosh(RDF(0.9))
0.45102681179626236*I

Note that this only works because RDF has no arccosh member function, so the computation is delegated to Pynac.

In this situation you could just fallback to

sage: RDF(0.9).acosh()
NaN

comment:23 in reply to: ↑ 20 Changed 16 months ago by vdelecroix

Replying to rws:

So why is this not a bug?!

sage: RR(-2).sqrt()
1.41421356237310*I

It is a bug.

comment:24 Changed 16 months ago by rws

Following your answer in Groups I think then, instead of calling x.func() in the symbolic function code, x.func(extend=True) should be called, or alternatively, have data in the Function when to call x.func() and when to call parent.complex_field(x).func().

comment:25 Changed 16 months ago by rws

  • Dependencies set to #24832

comment:26 Changed 16 months ago by rws

  • Branch set to u/rws/24428

comment:27 Changed 16 months ago by rws

  • Authors set to Ralf Stephan
  • Commit set to aa041a972d4b09a1e8b771b370e57f9f9ac19f7b
  • Dependencies changed from #24832 to #24832, pynac-0.7.17
  • Status changed from new to needs_review

This needs fixes from pynac-0.7.17. To fix RR(-1).log I'd suggest a similar change in functions/log.py to enable the fix of RR.log().


New commits:

ff8c67224832: Extend function domain with some symbolic function calls
3bfcc8824832: add doctest
aa041a924428: Numerical evaluation should return a complex number if applicable
Note: See TracTickets for help on using tickets.