Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Numerical evaluation should return a complex number if applicable #24428

Open
jdemeyer opened this issue Dec 25, 2017 · 32 comments
Open

Numerical evaluation should return a complex number if applicable #24428

jdemeyer opened this issue Dec 25, 2017 · 32 comments

Comments

@jdemeyer
Copy link
Contributor

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.

Depends on #24832

Dependencies: #24832, pynac-0.7.17

CC: @rwst @slel

Component: symbolics

Author: Ralf Stephan

Branch/Commit: u/rws/24428 @ aa041a9

Issue created by migration from https://trac.sagemath.org/ticket/24428

@jdemeyer jdemeyer added this to the sage-8.2 milestone Dec 25, 2017
@jdemeyer

This comment has been minimized.

@rwst
Copy link
Contributor

rwst commented Dec 25, 2017

comment:2

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

@rwst
Copy link
Contributor

rwst commented Dec 26, 2017

comment:3

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.

@rwst
Copy link
Contributor

rwst commented Dec 26, 2017

comment:4

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

@mantepse
Copy link
Contributor

comment:5

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

@rwst
Copy link
Contributor

rwst commented Dec 26, 2017

comment:6

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.

@mantepse
Copy link
Contributor

comment:7

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'

@mantepse
Copy link
Contributor

comment:8

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

@rwst
Copy link
Contributor

rwst commented Dec 27, 2017

comment:9

That is no surprise as I changed FP evaluation in the commit pynac/pynac@d0f66f9

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

@rwst
Copy link
Contributor

rwst commented Dec 27, 2017

comment:10

For example

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

(EDITED)

So one of arccos/arccosh should be changed.

@mantepse
Copy link
Contributor

comment:11

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 :-)

@slel

This comment has been minimized.

@slel
Copy link
Member

slel commented Feb 19, 2018

comment:12

Adding an example:

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

@jdemeyer

This comment has been minimized.

@jdemeyer jdemeyer changed the title Substitution should be the same as numerical evaluation Numerical evaluation should return a complex number if applicable Feb 20, 2018
@videlec
Copy link
Contributor

videlec commented Feb 24, 2018

comment:14

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.

@rwst
Copy link
Contributor

rwst commented Feb 24, 2018

comment:15

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 @videlec:

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.

@rwst
Copy link
Contributor

rwst commented Feb 24, 2018

comment:16

Replying to @slel:

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.

@videlec
Copy link
Contributor

videlec commented Feb 24, 2018

comment:17

Replying to @rwst:

Replying to @slel:

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

@rwst
Copy link
Contributor

rwst commented Feb 24, 2018

comment:18

Replying to @videlec:

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.

@rwst

This comment has been minimized.

@rwst
Copy link
Contributor

rwst commented Feb 24, 2018

comment:20

So why is this not a bug?!

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

@rwst
Copy link
Contributor

rwst commented Feb 24, 2018

@videlec
Copy link
Contributor

videlec commented Feb 24, 2018

comment:22

Replying to @rwst:

Replying to @slel:

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

@videlec
Copy link
Contributor

videlec commented Feb 24, 2018

comment:23

Replying to @rwst:

So why is this not a bug?!

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

It is a bug.

@rwst
Copy link
Contributor

rwst commented Feb 24, 2018

comment:24

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().

@rwst
Copy link
Contributor

rwst commented Feb 24, 2018

Dependencies: #24832

@rwst
Copy link
Contributor

rwst commented Feb 25, 2018

Branch: u/rws/24428

@rwst
Copy link
Contributor

rwst commented Feb 25, 2018

Author: Ralf Stephan

@rwst
Copy link
Contributor

rwst commented Feb 25, 2018

Changed dependencies from #24832 to #24832, pynac-0.7.17

@rwst
Copy link
Contributor

rwst commented Feb 25, 2018

Commit: aa041a9

@rwst
Copy link
Contributor

rwst commented Feb 25, 2018

comment:27

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

@kliem
Copy link
Contributor

kliem commented Dec 14, 2019

comment:28

Merge failed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants