Opened 11 years ago

Closed 8 years ago

# Make Airy functions symbolic

Reported by: Owned by: Oscar Gerardo Lazo Arjona Ralf Stephan major sage-6.6 symbolics Airy functions sd40.5 sd48 Karl-Dieter Crisman, Burcin Erocal, Benjamin Jones, Fredrik Johansson, Eviatar Bach Oscar Gerardo Lazo Arjona, Benjamin Jones, Douglas McNeil, Eviatar Bach, Ralf Stephan Eviatar Bach, Karl-Dieter Crisman, Burcin Erocal, Ralf Stephan, Jeroen Demeyer, Marc Mezzarobba N/A 2f6945a #12289, #17130

As discussed in sage-support.

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.

### comment:1 Changed 11 years ago by Oscar Gerardo Lazo Arjona

Component: PLEASE CHANGE → symbolics changed from tbd to Burcin Erocal

### comment:2 Changed 11 years ago by Oscar Gerardo Lazo Arjona

Owner: changed from Burcin Erocal to Oscar Gerardo Lazo Arjona

### comment:3 Changed 11 years ago by Oscar Gerardo Lazo Arjona

Description: modified (diff)

### comment:4 Changed 11 years ago by Oscar Gerardo Lazo Arjona

I've added a patch, which should do the job, but it has a few shortcomings:

1.-The resulting symbolic functions seem to remain on hold:

sage: airy_ai(1.0)
airy_ai(1.00000000000000)


You need to force it to evaluate:

sage: airy_ai(1.0).n()
0.135292416313


2.- This doesn't work:

sage: airy_ai(2.0).n(digits=100)
0.0349241304233


3.- There is no evaluation for airy_ai_prime or airy_bi_prime

4.- I'm not sure about how should the functions be called, some possible schemes are

{ai,bi,aip,bip} {ai,bai,aip,baip} {airy_ai,airy_bi,airy_ai_prime,airy_bi_prime}

And also whether the latex representation should be capitalized or not. I chose the third scheme, and capitalized typesetting.

### comment:5 Changed 11 years ago by Oscar Gerardo Lazo Arjona

Status: new → needs_review

### comment:6 Changed 11 years ago by Karl-Dieter Crisman

Cc: Fredrik Johansson added needs_review → needs_work Make Airy functions symbolical → Make Airy functions symbolic

Good start. A few responses.

• You'll need to have more methods. We should probably use mpmath to evaluate these functions in general, though using the SciPy version could be useful over RDF. See #11888 for a very recent example of this kind of thing and how to implement it.
• That should also fix precision issues, if properly done, I think.
• SciPy has evaluation for the airy primes. I think that Maxima has these too. (Cc:ing FJ in case he has plans to add these to mpmath.)
• We should have translations of these to Maxima and friends. That will probably help us name them. What do Mathematica and Maple call them?
• All methods, including initialization methods, need doctests. Many of the new symbolic functions added at wiki:symbolics/functions will have nice models to follow.

Thanks for getting us a start on this! That's great.

### comment:7 follow-up:  8 Changed 11 years ago by Fredrik Johansson

mpmath has the derivatives (as well as integrals) -- just use the optional 'derivative' parameter.

### comment:8 in reply to:  7 Changed 11 years ago by Karl-Dieter Crisman

mpmath has the derivatives (as well as integrals) -- just use the optional 'derivative' parameter.

Ok, I didn't realize that was what that parameter was about. Though in retrospect it seems obvious!

### comment:9 Changed 11 years ago by Oscar Gerardo Lazo Arjona

On second thought, I think it would be better to use the airy equation to calculate derivatives or order higher than 1. Like

sage: airy_ai(2,x)
x*airy_ai(x)
sage: airy_ai(3,x)
airy_ai(x)+x*airy_ai_prime(x)
sage: diff(airy_ai(x),x,2)
x*airy_ai(x)
sage: diff(airy_ai(x),x,3)
airy_ai(x)+x*airy_ai_prime(x)


which is very likey to be the way mpmath calculates higher order derivatives. Integrals however, would be returned as:

sage: airy_ai(-1,x)
airy_ai(-1,x)
sage: integral(airy_ai(x),x)
airy_ai(-1,x)


what do you think?

### comment:10 follow-up:  11 Changed 11 years ago by Oscar Gerardo Lazo Arjona

Status: needs_work → needs_review

I just added a new patch. The new version includes generalized derivatives, evaluation with mpmath, and special values of the functions and their derivatives. Just a one doubt, the coverage is:

oscar@oscar-netbook:~$sage -coverage airy.py SCORE /home/oscar/sage/my_patches/airy.py: 76% (20 of 26) Missing documentation: * __init__(self): * __init__(self): * __init__(self): * __init__(self): * __init__(self): * __init__(self): Possibly wrong (function name doesn't occur in doctests): * _derivative_(self, x, diff_param=None): * _eval_(self, x): * _evalf_(self, x, parent=None): * _derivative_(self, x, diff_param=None): * _eval_(self, x): * _evalf_(self, x, parent=None): * _derivative_(self, alpha, *args, **kwds): * _eval_(self, alpha, *args): * _evalf_(self, alpha, x, parent=None): * _derivative_(self, x, diff_param=None): * _eval_(self, x): * _evalf_(self, x, parent=None): * _derivative_(self, x, diff_param=None): * _eval_(self, x): * _evalf_(self, x, parent=None): * _derivative_(self, alpha, *args, **kwds): * _eval_(self, alpha, *args): * _evalf_(self, alpha, x, parent=None):  Is that important? ### comment:11 in reply to: 10 ; follow-ups: 12 14 Changed 11 years ago by Karl-Dieter Crisman Is that important? The second part isn't, as you are implicitly testing these 'hidden' functions. By the way, Examples::  should be EXAMPLES::  However, the first part (the initialization methods) is important for our coverage. You can just do a couple from the big examples that you have. Do these pass doctests? I'm surprised that sage: airy_ai_simple= FunctionAiryAiSimple()  would work when you run tests, assuming you didn't import FunctionAiryAiSimple into the global namespace. Anyway, overall on a quick glance this looks great; unfortunately, I won't have time to review it properly soon - hopefully someone else will, because you put a lot of great work into it and we should totally have these symbolic. Thanks! ### comment:12 in reply to: 11 Changed 11 years ago by Oscar Gerardo Lazo Arjona Do these pass doctests? I'm surprised that sage: airy_ai_simple= FunctionAiryAiSimple()  would work when you run tests, assuming you didn't import FunctionAiryAiSimple into the global namespace. It looks like they do: oscar@oscar-netbook:~$ sage -coverage airy.py
sage -t  "/home/oscar/sage/my_patches/airy.py"
[51.5 s]

----------------------------------------------------------------------
All tests passed!
Total time for all tests: 51.7 seconds


Why shouldn't they? I'll take care of the init functions.

### comment:13 Changed 11 years ago by Oscar Gerardo Lazo Arjona

I'm sorry, that should be:

oscar@oscar-netbook:~$sage -t airy.py sage -t "/home/oscar/sage/my_patches/airy.py" [51.5 s] ---------------------------------------------------------------------- All tests passed! Total time for all tests: 51.7 seconds  (the command was sage -t) ### comment:14 in reply to: 11 Changed 11 years ago by Karl-Dieter Crisman Status: needs_review → needs_work sage: airy_ai_simple= FunctionAiryAiSimple()  would work when you run tests, assuming you didn't import FunctionAiryAiSimple into the global namespace. I'm just surprised this doesn't cause trouble. On another note, this apparently does something weird when applied to 5.0.beta4. Namely, Sage won't start: ---> 65 from sage.functions.all import sin, cos ImportError: cannot import name sin Error importing ipy_profile_sage - perhaps you should run %upgrade? WARNING: Loading of ipy_profile_sage failed.  I'll spare you the rest of the traceback. I think there is some kind of circular import thing going on here, but I don't understand imports that well. Perhaps moving the import of airy to further down (after trig functions) would help - that's a totally uninformed guess, of course. ### Changed 11 years ago by Oscar Gerardo Lazo Arjona Attachment: trac_12455-symbolic_airy3.patch​ added ### comment:15 Changed 11 years ago by Oscar Gerardo Lazo Arjona Authors: → Oscar Gerardo Lazo Arjona needs_work → needs_review I just added a new patch, now with 100 % test coverage, but I still get this message: oscar@oscar-netbook:~$ sage -coverage /home/oscar/sage/my_patches/airy.py ----------------------------------------------------------------------
/home/oscar/sage/my_patches/airy.py
ERROR: Please add a TestSuite(s).run() doctest.
SCORE /home/oscar/sage/my_patches/airy.py: 100% (26 of 26)
----------------------------------------------------------------------


What is this TestSuite? thing? I also changed the order in which functions are initialized, so that airy_ai_general is initialized first. Doing it last made some symbolic operations, such as simplify not work (It said something about airy_ai requiring 2 arguments).

### comment:16 Changed 11 years ago by Benjamin Jones

Reviewers: → Benjamin Jones

Hi Oscar, I haven't looked over your patch very closely yet, but I plan to. One comment on first glance: in several places you call

return mpmath_utils.call(airy_bi_mpmath, x, derivative=1, parent=RR)


but I think we want to preserve the parent of x instead of forcing it to be RR. Importing the parent function from sage.structure.coerce using a different name like sage_parent (because as burcin pointed out in one of my tickets, parent is the name of the parameter in _evalf_) and doing something like

from sage.structure.coerce import parent as sage_parent
R = parent or sage_parent(x)
return mpmath_utils.call(airy_bi_mpmath, x, derivative=1, parent=R)


would do the trick I think.

Other comment: can we add conversions to Maxima along with the Mma conversion you included? Here is a link to the appropriate chapter in the manual: http://maxima.sourceforge.net/docs/manual/en/maxima_15.html#SEC80

### comment:17 Changed 11 years ago by Benjamin Jones

Status: needs_review → needs_work

The patch applies to 5.0.beta6 cleanly, but upon sage -br I get an import error ending with:

/home/jonesbe/sage/sage-5.0.beta6/local/lib/python2.7/site-packages/sage/gsl/dft.py in <module>()
63 from sage.rings.real_mpfr import RR
64 from sage.rings.all import I
---> 65 from sage.functions.all import sin, cos
66 from sage.gsl.fft import FastFourierTransform
67 from sage.gsl.dwt import WaveletTransform

ImportError: cannot import name sin
Error importing ipy_profile_sage - perhaps you should run %upgrade?


Seems like the same thing kcrisman ran into with the previous patch applied to 5.0.beta4.

### comment:18 Changed 11 years ago by Benjamin Jones

Work issues: → circular import, doctest failures

I found the cause of the problem in comment 17. There is a circular import going on because

from airy import airy_ai, airy_bi


occurs at the top of sage/functions/all.py. If you move it to the bottom of the file, sage starts just fine.

There a several doctests that fail, however. Oscar, I think that running sage -coverage only checks to see if there are doctest for every function, it doesn't actually run the tests. Use

sage -t devel/sage/sage/functions/airy.py


to run the tests and get feedback.

### comment:19 Changed 10 years ago by Benjamin Jones

Authors: Oscar Gerardo Lazo Arjona → Oscar Gerardo Lazo Arjona, Benjamin Jones Benjamin Jones

Here at SD 40.5, we're going to pick this ticket up, do the work, and get it into sage.

### comment:21 Changed 10 years ago by D.S. McNeil

Okay, I'm posting this for future reference. I've attempted to preserve as much as possible of the original code while switching to the "new-style" dictionary-argument .n() approach we're working on in #12289, but I'm setting it to "needs work" myself while we think through some of the consequences of the switch. Some of the _evalf_ stuff violates DRY and I'm not yet sure of the best way to deal with it.

As well, the patch currently segfaults in testing when it computes beta(0.5, 0.5) due to some problem with the current #12289 package, which I suspect is unrelated. But I think the new approach is beginning to take shape.

### Changed 10 years ago by D.S. McNeil

newstyle rewrite, version 1

### comment:22 Changed 10 years ago by D.S. McNeil

Dependencies: → #12289

### comment:23 Changed 10 years ago by D.S. McNeil

Oh, yeah: and #13028 was a significant nuisance while working on this.

### comment:25 Changed 9 years ago by Eviatar Bach

New patch, which should pass all doctests once #12289 is merged.

It makes the following changes:

• Changing an incorrect airy_bi identity
• Adding airy.py to the documentation
• Checking whether n is an integer (for some reason mpmath is able to evaluate numerically with non-integral n)
• Formatting changes
• Removing the parent's precision check because it wasn't used
• Removing the check for the parent keyword because Burcin told me to

Patchbot apply trac_12455_newstyle_airy.patch trac_12455_newstyle_airy2.patch

Last edited 9 years ago by Eviatar Bach (previous) (diff)

### comment:27 Changed 9 years ago by Karl-Dieter Crisman

Authors: Oscar Gerardo Lazo Arjona, Benjamin Jones → Oscar Gerardo Lazo Arjona, Benjamin Jones, Eviatar Bach modified (diff) needs_work → needs_review

Needs review, then? Note that I had to rebase #12289.

### comment:28 Changed 9 years ago by Karl-Dieter Crisman

Description: modified (diff) → Eviatar Bach, Karl-Dieter Crisman

Patchbot apply trac_12455-newstyle-airy-rebase.patch and trac_12455_newstyle_airy2.patch

I don't know that either of the needs work issues are still there...

### comment:29 Changed 9 years ago by Karl-Dieter Crisman

This needs a trivial extra patch to fix

sage -t sage/functions/special.py
**********************************************************************
File "sage/functions/special.py", line 389, in sage.functions.special._init
Failed example:
spherical_harmonic(3,2,1,2)
Expected:
15/4*sqrt(7/30)*e^(4*I)*sin(1)^2*cos(1)/sqrt(pi)
Got:
15/4*sqrt(7/30)*cos(1)*e^(4*I)*sin(1)^2/sqrt(pi)
**********************************************************************


which is really a rebase issue off of #9880, no worries.

### comment:30 Changed 9 years ago by Karl-Dieter Crisman

Okay, a little more rebasing for #12289. Patches coming up.

### comment:31 Changed 9 years ago by Karl-Dieter Crisman

Description: modified (diff)

Apply trac_12455-newstyle-airy-rebase.patch and trac_12455-newstyle-airy2-rebase.patch

### comment:32 Changed 9 years ago by Karl-Dieter Crisman

Eviatar, how much would you say still needs to be reviewed? (In the sense that you have not yet given it positive review.) Just your patch?

(I'm running doctests now.)

### comment:33 Changed 9 years ago by Karl-Dieter Crisman

I made some mistake while rebasing - one moment.

### comment:34 Changed 9 years ago by Eviatar Bach

I think I was fairly thorough, but I haven't positive-reviewed since my patch hasn't been looked at by someone else.

### comment:35 Changed 9 years ago by Karl-Dieter Crisman

Okay, now I have to fix something in the other one... sigh. Final version coming soon, it does pass tests!

### comment:36 Changed 9 years ago by Karl-Dieter Crisman

Patchbot, apply trac_12455-newstyle-airy-rebase.patch and trac_12455-newstyle-airy2-rebase.patch

### comment:37 Changed 9 years ago by Karl-Dieter Crisman

(for some reason mpmath is able to evaluate numerically with non-integral n)

But this is even in the documentation you wrote!

Return the \alpha-th order fractional derivative with respect to z.


"Fractional", right? So ... ? Plus, you do not make this change in airy_bi, only airy_ai, for some reason.

Another question:

sage: (plot(airy_bi(x), (x, -10, 5)) +\


Are we allowing this kind of continuation in doctests still? I can't remember but seem to recall something about this being a problem.

Otherwise this all seems fine.

Last edited 9 years ago by Karl-Dieter Crisman (previous) (diff)

### comment:38 Changed 9 years ago by Eviatar Bach

Status: needs_review → needs_work

I didn't write it. But looking at the mpmath documentation, it should work. It didn't work for most values before my patch anyway, but I'll see what I can do.

### comment:39 Changed 9 years ago by Karl-Dieter Crisman

I didn't write it.

Good point, sorry! In fact, it's just copied from the mpmath doc. But now you have taken charge :-) Unfortunately, I can't find a lot of independent implementations of this...

### comment:40 Changed 9 years ago by Burcin Erocal

Authors: Oscar Gerardo Lazo Arjona, Benjamin Jones, Eviatar Bach → Oscar Gerardo Lazo Arjona, Benjamin Jones, D. S. McNeil, Eviatar Bach modified (diff) Eviatar Bach, Karl-Dieter Crisman → Eviatar Bach, Karl-Dieter Crisman, Burcin Erocal

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.

### comment:41 Changed 9 years ago by Jeroen Demeyer

Milestone: sage-5.11 → sage-5.12

### comment:42 Changed 9 years ago by Eviatar Bach

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?

### comment:43 Changed 9 years ago by Benjamin Jones

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.

Last edited 9 years ago by Benjamin Jones (previous) (diff)

### comment:44 Changed 9 years ago by For batch modifications

Milestone: sage-6.1 → sage-6.2

### comment:45 Changed 8 years ago by For batch modifications

Milestone: sage-6.2 → sage-6.3

### comment:46 Changed 8 years ago by Ralf Stephan

Branch: → u/rws/make_airy_functions_symbolic

### comment:47 Changed 8 years ago by git

Commit: → 22731c9d5b976301d282e3b48e259f5f13d4b8bd

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

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

### comment:48 Changed 8 years ago by Ralf Stephan

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


### comment:49 Changed 8 years ago by git

Commit: 22731c9d5b976301d282e3b48e259f5f13d4b8bd → 515e343290448d2bbe5e75f88accea814be9215f

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

### comment:50 Changed 8 years ago by Ralf Stephan

Authors: Oscar Gerardo Lazo Arjona, Benjamin Jones, D. S. McNeil, Eviatar Bach → Oscar Gerardo Lazo Arjona, Benjamin Jones, D. S. McNeil, Eviatar Bach, Ralf Stephan needs_work → needs_review circular import, doctest failures

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

### comment:51 Changed 8 years ago by For batch modifications

Milestone: sage-6.3 → sage-6.4

### comment:52 Changed 8 years ago by Frédéric Chapoton

Status: needs_review → needs_work

one failing doctest, see patchbot report

### comment:53 Changed 8 years ago by git

Commit: 515e343290448d2bbe5e75f88accea814be9215f → 8e76828f9f5c988b0429230cd1050c6086fb730b

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

### comment:54 Changed 8 years ago by Ralf Stephan

Owner: changed from Oscar Gerardo Lazo Arjona to Ralf Stephan

### comment:55 Changed 8 years ago by Ralf Stephan

Owner: changed from Ralf Stephan to Oscar Gerardo Lazo Arjona

### comment:56 Changed 8 years ago by Ralf Stephan

Status: needs_work → needs_review

### comment:57 Changed 8 years ago by Frédéric Chapoton

Branch: u/rws/make_airy_functions_symbolic → public/ticket/12455 8e76828f9f5c988b0429230cd1050c6086fb730b → 95526065b2703e42378838747055bb6bbc409402

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

### comment:58 Changed 8 years ago by git

Commit: 95526065b2703e42378838747055bb6bbc409402 → 7102227617bf289b2620636e397b92c081f9715a

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

 ​7102227 Merge with 6.4.beta4

### comment:59 Changed 8 years ago by git

Commit: 7102227617bf289b2620636e397b92c081f9715a → 5b731d32d7aec6bdb88acf777253c777ccb6f509

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

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

### comment:60 Changed 8 years ago by git

Commit: 5b731d32d7aec6bdb88acf777253c777ccb6f509 → 39deab4d13c17120737a875a4b2744a67ce61227

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

 ​39deab4 trac #12455 formatting doc and code

### comment:61 Changed 8 years ago by Ralf Stephan

Reviewers: Eviatar Bach, Karl-Dieter Crisman, Burcin Erocal → Eviatar Bach, Karl-Dieter Crisman, Burcin Erocal, Ralf Stephan needs_review → needs_work
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


### comment:62 Changed 8 years ago by git

Commit: 39deab4d13c17120737a875a4b2744a67ce61227 → fbd6c357dfdba94d7630d6f2becb411f6169f004

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

 ​fbd6c35 trac #12455 plot output

### comment:63 Changed 8 years ago by Frédéric Chapoton

Status: needs_work → needs_review

### comment:64 Changed 8 years ago by Jeroen Demeyer

Description: modified (diff)

### comment:65 Changed 8 years ago by Jeroen Demeyer

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

### comment:66 follow-up:  69 Changed 8 years ago by Jeroen Demeyer

Status: needs_review → needs_info

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?

### comment:67 Changed 8 years ago by Jeroen Demeyer

Status: needs_info → needs_work

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


### comment:68 Changed 8 years ago by Jeroen Demeyer

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?

### comment:69 in reply to:  66 Changed 8 years ago by Ralf Stephan

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

### comment:70 Changed 8 years ago by git

Commit: fbd6c357dfdba94d7630d6f2becb411f6169f004 → 61847c43253572e53c6ed25fe208f4eb19fd1b87

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

### comment:71 Changed 8 years ago by Ralf Stephan

Status: needs_work → needs_review

### comment:72 Changed 8 years ago by git

Commit: 61847c43253572e53c6ed25fe208f4eb19fd1b87 → aac86a20bbb84df905e4000dfb79f125247cf85b

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

### comment:73 Changed 8 years ago by Ralf Stephan

Dependencies: #12289 → #12289, #17130

### comment:74 Changed 8 years ago by Karl-Dieter Crisman

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.

### comment:75 Changed 8 years ago by Frédéric Chapoton

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

### comment:76 Changed 8 years ago by git

Commit: aac86a20bbb84df905e4000dfb79f125247cf85b → 070e72855b15c138e9d22ae8325633d84ef49f69

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

### comment:77 Changed 8 years ago by Ralf Stephan

Milestone: sage-6.4 → sage-6.6 changed from Oscar Gerardo Lazo Arjona to Ralf Stephan

### comment:78 Changed 8 years ago by Marc Mezzarobba

x should be z in the differential equation

### comment:79 Changed 8 years ago by Marc Mezzarobba

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.

### comment:80 follow-ups:  81  84 Changed 8 years ago by Marc Mezzarobba

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_?

### comment:81 in reply to:  80 Changed 8 years ago by Marc Mezzarobba

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

Scratch that, I misread the code.

### comment:82 Changed 8 years ago by git

Commit: 070e72855b15c138e9d22ae8325633d84ef49f69 → 91530d08a7a37bc03876962c1a1ae0bca10bfd70

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

 ​91530d0 Ai/Bi: doc fixes and clarifications

### comment:83 Changed 8 years ago by Marc Mezzarobba

One last thing: IMO hold_derivative should be True by default in airy_ai and airy_bi, except perhaps when the differentiation order is one. But I don't know if that's consistent with the way things are done in other parts of sage symbolics. Thoughts?

### comment:84 in reply to:  80 ; follow-up:  86 Changed 8 years ago by Ralf Stephan

x should be z in the differential equation

Strange typo.

• 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.

A quick look at old and new code shows 1. ValueError, 2. NotImplementedError, 3. assert. So, this needs to be unified to NotImplementedError.

One last thing: IMO hold_derivative should be True by default in airy_ai and airy_bi, except perhaps when the differentiation order is one. But I don't know if that's consistent with the way things are done in other parts of sage symbolics. Thoughts?

I believe the code of this ticket is the only one that has such a parameter, so please feel free to improve it.

I have looked at your patch and it's fine, so if you think the rest is OK please set positive.

### comment:85 Changed 8 years ago by git

Commit: 91530d08a7a37bc03876962c1a1ae0bca10bfd70 → 2f6945af73219458fb0a2745e2df9e2eea79b055

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

 ​347cd08 #12455 Airy functions: doc fixes and clarifications ​6aaf2da #12455 Airy funs: raise NotImplementedError on diff(airy_?i(α,x),α) ​c80be63 #12455 Airy funs: hold_derivative by default ​2f6945a #12455 Airy functions: avoid calling deprecated function

### comment:86 in reply to:  84 Changed 8 years ago by Marc Mezzarobba

Description: modified (diff) Eviatar Bach, Karl-Dieter Crisman, Burcin Erocal, Ralf Stephan → Eviatar Bach, Karl-Dieter Crisman, Burcin Erocal, Ralf Stephan, Jeroen Demeyer, Marc Mezzarobba

• 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.

A quick look at old and new code shows 1. ValueError, 2. NotImplementedError, 3. assert. So, this needs to be unified to NotImplementedError.

One last thing: IMO hold_derivative should be True by default in airy_ai and airy_bi, except perhaps when the differentiation order is one. But I don't know if that's consistent with the way things are done in other parts of sage symbolics. Thoughts?

I believe the code of this ticket is the only one that has such a parameter, so please feel free to improve it.

Both done (now rather than later to avoid breaking the interface).

I have looked at your patch and it's fine, so if you think the rest is OK please set positive.

I'm okay with everything prior to my commits. Could you (or someone else) have a quick look at my last changes and set the ticket to positive review? Thanks!

### comment:87 Changed 8 years ago by Ralf Stephan

Status: needs_review → positive_review

Is fine and passes tests in functions/. Thank you too.

### comment:88 Changed 8 years ago by Volker Braun

Branch: public/ticket/12455 → 2f6945af73219458fb0a2745e2df9e2eea79b055 → fixed positive_review → closed

### comment:89 Changed 7 years ago by Karl-Dieter Crisman

Authors: Oscar Gerardo Lazo Arjona, Benjamin Jones, D. S. McNeil, Eviatar Bach, Ralf Stephan → Oscar Gerardo Lazo Arjona, Benjamin Jones, Douglas McNeil, Eviatar Bach, Ralf Stephan 2f6945af73219458fb0a2745e2df9e2eea79b055
Note: See TracTickets for help on using tickets.