Make Airy functions symbolic

38968367-17c9-42b1-b82d-c1adf20431c2 commented 12 years ago

Currently sage can evaluate airy functions numerically:

sage: airy_ai(1.4)
0.0820380498076

but it doesn't work symbolically

sage: airy_ai(x)
...
TypeError: Cannot evaluate symbolic expression to a numeric value.

We should make it symbolical for both airy_ai and airy_bi, as well as their derivatives.

Depends on #12289 Depends on #17130

CC: @kcrisman @burcin @benjaminfjones @fredrik-johansson @eviatarbach

Component: symbolics

Keywords: Airy functions sd40.5 sd48

Author: Oscar Gerardo Lazo Arjona, Benjamin Jones, Douglas McNeil, Eviatar Bach, Ralf Stephan

Branch: 2f6945a

Reviewer: Eviatar Bach, Karl-Dieter Crisman, Burcin Erocal, Ralf Stephan, Jeroen Demeyer, Marc Mezzarobba

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

burcin commented 11 years ago

Changed reviewer from Eviatar Bach, Karl-Dieter Crisman to Eviatar Bach, Karl-Dieter Crisman, Burcin Erocal

burcin commented 11 years ago

comment:40

attachment: trac_12455-airy_review.patch fixes a couple of problems:

airy_{a,b}i_general should evaluate to airy_{a,b}i_simple, airy_{a,b}i_prime, etc. depending on the first parameter
differentiating airy_{a,b}i_general in the first parameter is not allowed
_evalf_ should not raise errors for unrecognized algorithm argument

This is still needs work because we use indirect doctests everywhere. There is no reason to go through the airy_{a,b}i wrapper functions. We should call the symbolic functions directly in the doctests. There are also a lot of code paths that are not tested. I changed the fractional order behavior, but didn't need to change any doctests.

It would be great if someone else can fix/add doctests. I will move on to something else now.

burcin commented 11 years ago

Changed author from Oscar Gerardo Lazo Arjona, Benjamin Jones, Eviatar Bach to Oscar Gerardo Lazo Arjona, Benjamin Jones, D. S. McNeil, Eviatar Bach

eviatarbach commented 11 years ago

comment:42

This patch depends on a new release of Pynac and other fixes to make the algorithm keyword work sensibly. Also, I don't see much point in having the Maxima implementation as an option, considering the fact that it doesn't accept complex input, doesn't have arbitrary precision, and is slower than mpmath (doesn't appear to scale better either).

I'd hate to delay this patch longer, especially considering that the Airy functions are probably among the most-used special functions. Could we remove the Maxima option for now and perhaps add it in the future, maybe along with a patch that adds a SciPy option as suggested in one of the comments?

benjaminfjones commented 11 years ago

comment:43

I'm fine with removing Maxima from the options for numerical evaluation (numerical is not generally where Maxima shines). I agree with @burcin, though, that indirect doctests should be fixed wherever possible and become direct tests and that the branch coverage should be higher.

What exactly is the issue with having the Maxima option? Is it the limited precision and domain? We could always check that inputs are okay whenever Maxima is selected and raise an exception otherwise.

rwst commented 10 years ago

Branch: u/rws/make_airy_functions_symbolic

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 10 years ago

Commit: 22731c9

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 10 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

`aeb0800`	`12455: remove algorithm=maxima`
`22731c9`	`12455: implement algorithm='scipy'`

rwst commented 10 years ago

comment:48

Replying to @benjaminfjones:

What exactly is the issue with having the Maxima option? Is it the limited precision and domain? We could always check that inputs are okay whenever Maxima is selected and raise an exception otherwise.

It's useless, especially when scipy is implemented as algorithm. OTOH, it introduces more code paths to test and deepens a dependency that is unwanted.

Oh, and scipy is a bit faster:

sage: timeit("airy_bi(2).n(algorithm='maxima')")
625 loops, best of 3: 685 µs per loop
sage: timeit("airy_bi(2).n(algorithm='scipy')")
625 loops, best of 3: 124 µs per loop

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 10 years ago

Changed commit from 22731c9 to 515e343

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 10 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

`a88d035`	`12455: fix indirect and plot doctests`
`f91a3c2`	`12455: hold_derivate only implemented in wrapper functions`
`515e343`	`12455: add small index to documentation head`

rwst commented 10 years ago

Changed author from Oscar Gerardo Lazo Arjona, Benjamin Jones, D. S. McNeil, Eviatar Bach to Oscar Gerardo Lazo Arjona, Benjamin Jones, D. S. McNeil, Eviatar Bach, Ralf Stephan

rwst commented 10 years ago

comment:50

Everything was reviewed up to my recent changes:

merged develop
removal of maxima and inclusion of scipy for numerics
replacement of indirect with direct doctests (still 100% coverage)
removal of hold_derivate keyword from airy_ai/bi_general() where it wasn't implemented (it is still in airy_ai/bi() wrapper functions
small index at the top of documentation for orientation, because of confusing presentation order of functions, which cannot be controlled, sadly

So please review.

rwst commented 10 years ago

Changed work issues from circular import, doctest failures to none

fchapoton commented 10 years ago

comment:52

one failing doctest, see patchbot report

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 10 years ago

Changed commit from 515e343 to 8e76828

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 10 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

`1715892`	`Merge branch 'develop' into t/12455/make_airy_functions_symbolic`
`8e76828`	`12455: trigger doctest with MaximaFunction as intended`

fchapoton commented 10 years ago

Changed branch from u/rws/make_airy_functions_symbolic to public/ticket/12455

fchapoton commented 10 years ago

comment:57

Rebased on 6.4.beta3, and add a few # rel tol to the scipy results.

New commits:

`c3449e8`	`Merge branch 'u/rws/make_airy_functions_symbolic' and 6.4.beta3`
`9552606`	`trac #12455 fixing doctests by adding rel tol and changing imports`

fchapoton commented 10 years ago

Changed commit from 8e76828 to 9552606

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 10 years ago

Changed commit from 9552606 to 7102227

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 10 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

7102227 Merge with 6.4.beta4

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 10 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

5b731d3 trac #12455 change use of Airy as example of maxima function

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 10 years ago

Changed commit from 7102227 to 5b731d3

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 10 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

39deab4 trac #12455 formatting doc and code

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 10 years ago

Changed commit from 5b731d3 to 39deab4

rwst commented 10 years ago

Changed reviewer from Eviatar Bach, Karl-Dieter Crisman, Burcin Erocal to Eviatar Bach, Karl-Dieter Crisman, Burcin Erocal, Ralf Stephan

rwst commented 10 years ago

comment:61

sage -t --long src/sage/functions/airy.py
**********************************************************************
File "src/sage/functions/airy.py", line 404, in sage.functions.airy.airy_ai
Failed example:
    plot(airy_ai(x), (x, -10, 5)) + plot(airy_ai_prime(x),
     (x, -10, 5), color='red')
Expected nothing
Got:
    Graphics object consisting of 2 graphics primitives
**********************************************************************
File "src/sage/functions/airy.py", line 799, in sage.functions.airy.airy_bi
Failed example:
    plot(airy_bi(x), (x, -10, 5)) + plot(airy_bi_prime(x),
     (x, -10, 5), color='red')
Expected nothing
Got:
    Graphics object consisting of 2 graphics primitives

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 10 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

fbd6c35 trac #12455 plot output

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 10 years ago

Changed commit from 39deab4 to fbd6c35

jdemeyer commented 10 years ago

Description changed:

--- 
+++ 
@@ -35,11 +35,3 @@

We should make it symbolical for both airy_ai and airy_bi, as well as their derivatives.

- -Apply

- attachment: trac_12455-newstyle-airy-rebase.patch - attachment: trac_12455-newstyle-airy2-rebase.patch -* [attachment: trac_12455-airy_review.patch]

jdemeyer commented 10 years ago

comment:65

Just to make you aware: this conflicts with #17130. When either of these tickets gets positive_review, the other should be rebased.

jdemeyer commented 10 years ago

comment:66

And why the change to def hypergeometric_U? The new code seems quite hackish, why is it needed and how does it relate to this ticket?

jdemeyer commented 10 years ago

comment:67

These should be an error:

sage: airy_ai(3).n(algorithm='scipy', prec=200)
0.006591139357460719
sage: airy_ai(3).n(algorithm='whatever')
0.00659113935746072

And I don't like this either (parent should be RR):

sage: parent(airy_ai(3).n(algorithm='scipy'))
Real Double Field

jdemeyer commented 10 years ago

comment:68

What's the point of

if len(args) > 0:
   raise TypeError(("symbolic function airy_ai takes at most 3 arguments "
      "({} given)").format(len(args) + 3))

why not simply remove *args completely then?

rwst commented 10 years ago

comment:69

Replying to @jdemeyer:

And why the change to def hypergeometric_U? The new code seems quite hackish, why is it needed and how does it relate to this ticket?

I think that unrelated patch can be safely removed because scipy evaluation is replaced by mpmath in #14896 (of course hoping that that gets reviewed at some time).

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 10 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

`15e7e34`	`Merge branch 'develop' into t/12455/public/ticket/12455`
`2c0931c`	`12455: remove fishy code in hypergeometric_U()`
`b4213d3`	`12455: with algorithm=scipy return reals for real argument`
`b0841c9`	`12455: catch attempts to exceed precision with algorithm=scipy`
`484a615`	`12455: catch unknown evalf algorithms`
`58e006c`	`12455: remove superfluous args and check`
`870e003`	`12455: fix plot doctests`
`61847c4`	`Merge branch 'public/ticket/12455' of trac.sagemath.org:sage into t/12455/public/ticket/12455`

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 10 years ago

Changed commit from fbd6c35 to 61847c4

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

`6d10729`	`Simplify numerical evaluation of BuiltinFunctions`
`b6e1ed4`	`Merge remote-tracking branches 'origin/u/jdemeyer/ticket/17131' and 'origin/u/jdemeyer/ticket/17133' into ticket/17130`
`382f97a`	`Merge branch 'u/jdemeyer/ticket/17130' of trac.sagemath.org:sage into 6.5beta1`
`7265989`	`17130: reviewer's patch: fix typo`
`c47dbd4`	`Fix Trac #17328 again in a better way`
`a486db2`	`Call the factorial() method of an Integer`
`9d3cbbd`	`Fix numerical noise`
`abab222`	`Fix more numerical noise`
`01614f7`	`Merge branch 'u/jdemeyer/ticket/17130' of trac.sagemath.org:sage into t/12455/public/ticket/12455`
`aac86a2`	`12455: simplifications due to trac 17130`

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from 61847c4 to aac86a2

rwst commented 9 years ago

Changed dependencies from #12289 to #12289, #17130

kcrisman commented 9 years ago

comment:74

Frédéric, how much of this did you positively review up to now? In principle you and rws can review each other's contributions here. It would take me a while to get up to speed here.

fchapoton commented 9 years ago

comment:75

a typo in line 5 of the doc:

Airy functions are solutions to the differential equation `f''(z) +f(z)x=0`.

The differential equation is wrong, should be f'' - x f = 0

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from aac86a2 to 070e728

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

`3cac775`	`Merge branch 'develop' into t/12455/public/ticket/12455`
`070e728`	`12455: doc typos`

mezzarobba commented 9 years ago

comment:78

x should be z in the differential equation

mezzarobba commented 9 years ago

comment:79

What exactly still needs review here?

The code looks pretty good to me overall, but there are lots of things I don't understand in detail. (For a simple example, what is the motivation for splitting the implementation in so many unrelated classes?) And yet I'm tempted to give this ticket positive review (leaving possible remaining issues for later), since many other people have looked at it and it is clearly an improvement over what sage currently has.

mezzarobba commented 9 years ago

comment:80

More nitpicking, not necessarily for this ticket:

Raising a ValueError in FunctionAiryAiGeneral._derivative_ when diff_param == 0 may not be the most appropriate, since (if I'm not mistaken) the partial derivative would make sense mathematically.
Is it necessary to special-case alpha == 0 etc. in FunctionAiryAiGeneral._evalf_?

mezzarobba commented 9 years ago

comment:81

Replying to @mezzarobba:

Is it necessary to special-case alpha == 0 etc. in FunctionAiryAiGeneral._evalf_?

Scratch that, I misread the code.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from 070e728 to 91530d0

sagemath / sage

Make Airy functions symbolic #12455

We should make it symbolical for both airy_ai and airy_bi, as well as their derivatives.

- -Apply