Opened 3 years ago
Last modified 7 months ago
#24428 needs_work defect
Numerical evaluation should return a complex number if applicable
Reported by:  jdemeyer  Owned by:  

Priority:  major  Milestone:  sage8.2 
Component:  symbolics  Keywords:  
Cc:  rws, slelievre  Merged in:  
Authors:  Ralf Stephan  Reviewers:  
Report Upstream:  N/A  Work issues:  
Branch:  u/rws/24428 (Commits, GitHub, GitLab)  Commit:  aa041a972d4b09a1e8b771b370e57f9f9ac19f7b 
Dependencies:  #24832, pynac0.7.17  Stopgaps: 
Description (last modified by )
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 (29)
comment:1 Changed 3 years ago by
 Description modified (diff)
comment:2 Changed 3 years ago by
comment:3 Changed 3 years ago by
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 3 years ago by
Note that RBF(0.9).arccosh()
returns nan as well.
comment:5 Changed 3 years ago by
Does an expression in SR
(like acosh
) come with a notion of domain and codomain?
comment:6 Changed 3 years ago by
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 errorit would enable much better random testing.
comment:7 Changed 3 years ago by
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: 20170911'
comment:8 Changed 3 years ago by
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 3 years ago by
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 3 years ago by
For example
sage: arccos(1.1) NaN sage: arccos(x).subs(x=1.1) NaN
(EDITED)
So one of arccos/arccosh
should be changed.
comment:11 Changed 3 years ago by
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 followup: ↓ 16 Changed 3 years ago by
 Cc slelievre added
 Description modified (diff)
Adding an example:
sage: arccosh(RDF(0.9)) 0.45102681179626236*I
comment:13 Changed 3 years ago by
 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 followup: ↓ 15 Changed 3 years ago by
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 3 years ago by
With pynac0.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 ofsage: 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 ; followup: ↓ 17 Changed 3 years ago by
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 ; followup: ↓ 18 Changed 3 years ago by
comment:18 in reply to: ↑ 17 Changed 3 years ago by
Replying to vdelecroix:
sage: arccosh(RDF(0.9)) 0.45102681179626236*INote 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 arc
s (arcus) to the a
s 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.
comment:19 Changed 3 years ago by
 Description modified (diff)
comment:20 followups: ↓ 22 ↓ 23 Changed 3 years ago by
So why is this not a bug?!
sage: RR(2).sqrt() 1.41421356237310*I
comment:21 Changed 3 years ago by
comment:22 in reply to: ↑ 20 Changed 3 years ago by
comment:23 in reply to: ↑ 20 Changed 3 years ago by
comment:24 Changed 3 years ago by
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 3 years ago by
 Dependencies set to #24832
comment:26 Changed 3 years ago by
 Branch set to u/rws/24428
comment:27 Changed 3 years ago by
 Commit set to aa041a972d4b09a1e8b771b370e57f9f9ac19f7b
 Dependencies changed from #24832 to #24832, pynac0.7.17
 Status changed from new to needs_review
This needs fixes from pynac0.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:
ff8c672  24832: Extend function domain with some symbolic function calls

3bfcc88  24832: add doctest

aa041a9  24428: Numerical evaluation should return a complex number if applicable

comment:29 Changed 7 months ago by
 Priority changed from critical to major
I'm not decided on which result is correct. But see also #15344.