Opened 3 years ago
Last modified 8 days ago
#12455 needs_review enhancement
Make Airy functions symbolic
Buildbot: Queued
Reported by: | olazo | Owned by: | olazo |
---|---|---|---|
Priority: | major | Milestone: | sage-6.4 |
Component: | symbolics | Keywords: | Airy functions sd40.5 sd48 |
Cc: | kcrisman, burcin, benjaminfjones, fredrik.johansson, eviatarbach | Merged in: | |
Authors: | Oscar Gerardo Lazo Arjona, Benjamin Jones, D. S. McNeil, Eviatar Bach, Ralf Stephan | Reviewers: | Eviatar Bach, Karl-Dieter Crisman, Burcin Erocal |
Report Upstream: | N/A | Work issues: | |
Branch: | u/rws/make_airy_functions_symbolic (Commits) | Commit: | 8e76828f9f5c988b0429230cd1050c6086fb730b |
Dependencies: | #12289 | Stopgaps: |
Description (last modified by burcin)
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 Traceback (most recent call last) /home/oscar/Escritorio/tesis/calculos/<ipython console> in <module>() /home/oscar/sage/sage-4.7.1/local/lib/python2.6/site-packages/sage/functions/special.pyc in airy_ai(x) 621 """ 622 _init() --> 623 return RDF(meval("airy_ai(%s)"%RDF(x))) 624 625 def airy_bi(x): /home/oscar/sage/sage-4.7.1/local/lib/python2.6/site-packages/sage/structure/parent.so in sage.structure.parent.Parent.__call__ (sage/structure/parent.c:7102)() /home/oscar/sage/sage-4.7.1/local/lib/python2.6/site-packages/sage/structure/coerce_maps.so in sage.structure.coerce_maps.NamedConvertMap._call_ (sage/structure/coerce_maps.c:4221)() /home/oscar/sage/sage-4.7.1/local/lib/python2.6/site-packages/sage/symbolic/expression.so in sage.symbolic.expression.Expression._real_double_ (sage/symbolic/expression.cpp:5391)() /home/oscar/sage/sage-4.7.1/local/lib/python2.6/site-packages/sage/symbolic/expression.so in sage.symbolic.expression.Expression._eval_self (sage/symbolic/expression.cpp:4898)() 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.
Apply
Attachments (8)
Change History (64)
comment:1 Changed 3 years ago by olazo
- Component changed from PLEASE CHANGE to symbolics
- Owner changed from tbd to burcin
comment:2 Changed 3 years ago by olazo
- Owner changed from burcin to olazo
comment:3 Changed 3 years ago by olazo
- Description modified (diff)
Changed 3 years ago by olazo
comment:4 Changed 3 years ago by olazo
comment:5 Changed 3 years ago by olazo
- Status changed from new to needs_review
comment:6 Changed 3 years ago by kcrisman
- Cc fredrik.johansson added
- Status changed from needs_review to needs_work
- Summary changed from Make Airy functions symbolical to 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 3 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 3 years ago by kcrisman
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 3 years ago by olazo
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?
Changed 3 years ago by olazo
comment:10 follow-up: ↓ 11 Changed 3 years ago by olazo
- Status changed from needs_work to 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 3 years ago by kcrisman
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 3 years ago by olazo
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 3 years ago by olazo
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 3 years ago by kcrisman
- Status changed from needs_review to 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 3 years ago by olazo
comment:15 Changed 3 years ago by olazo
- Status changed from needs_work to 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 3 years ago by benjaminfjones
- Reviewers set to 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 3 years ago by benjaminfjones
- Status changed from needs_review to 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? WARNING: Loading of ipy_profile_sage failed.
Seems like the same thing kcrisman ran into with the previous patch applied to 5.0.beta4.
comment:18 Changed 2 years ago by benjaminfjones
- Work issues set to 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 2 years ago by benjaminfjones
- Reviewers Benjamin Jones deleted
Here at SD 40.5, we're going to pick this ticket up, do the work, and get it into sage.
comment:20 Changed 2 years ago by benjaminfjones
- Keywords sd40.5 added
comment:21 Changed 2 years ago by dsm
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.
comment:22 Changed 2 years ago by dsm
- Dependencies set to #12289
comment:23 Changed 2 years ago by dsm
Oh, yeah: and #13028 was a significant nuisance while working on this.
comment:24 Changed 15 months ago by kcrisman
- Cc eviatarbach added
Changed 15 months ago by eviatarbach
comment:25 Changed 15 months ago by eviatarbach
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
comment:26 Changed 15 months ago by eviatarbach
- Keywords sd48 added
comment:27 Changed 15 months ago by kcrisman
- Description modified (diff)
- Status changed from needs_work to needs_review
Needs review, then? Note that I had to rebase #12289.
comment:28 Changed 15 months ago by kcrisman
- Description modified (diff)
- Reviewers set to 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 15 months ago by kcrisman
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 15 months ago by kcrisman
Okay, a little more rebasing for #12289. Patches coming up.
comment:31 Changed 15 months ago by kcrisman
- Description modified (diff)
Apply trac_12455-newstyle-airy-rebase.patch and trac_12455-newstyle-airy2-rebase.patch
comment:32 Changed 15 months ago by kcrisman
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 15 months ago by kcrisman
I made some mistake while rebasing - one moment.
comment:34 Changed 15 months ago by eviatarbach
I think I was fairly thorough, but I haven't positive-reviewed since my patch hasn't been looked at by someone else.
Changed 15 months ago by kcrisman
comment:35 Changed 15 months ago by kcrisman
Okay, now I have to fix something in the other one... sigh. Final version coming soon, it does pass tests!
Changed 15 months ago by kcrisman
comment:36 Changed 15 months ago by kcrisman
Patchbot, apply trac_12455-newstyle-airy-rebase.patch and trac_12455-newstyle-airy2-rebase.patch
comment:37 Changed 15 months ago by kcrisman
(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.
comment:38 Changed 15 months ago by eviatarbach
- Status changed from needs_review to 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 15 months ago by kcrisman
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...
Changed 15 months ago by burcin
comment:40 Changed 15 months ago by burcin
- Description modified (diff)
- Reviewers changed from Eviatar Bach, Karl-Dieter Crisman to 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 13 months ago by jdemeyer
- Milestone changed from sage-5.11 to sage-5.12
comment:42 Changed 12 months ago by eviatarbach
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 12 months ago by benjaminfjones
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.
comment:44 Changed 7 months ago by vbraun_spam
- Milestone changed from sage-6.1 to sage-6.2
comment:45 Changed 4 months ago by vbraun_spam
- Milestone changed from sage-6.2 to sage-6.3
comment:46 Changed 7 weeks ago by rws
- Branch set to u/rws/make_airy_functions_symbolic
comment:47 Changed 6 weeks ago by git
- Commit set to 22731c9d5b976301d282e3b48e259f5f13d4b8bd
comment:48 Changed 6 weeks ago by rws
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
comment:49 Changed 6 weeks ago by git
- Commit changed from 22731c9d5b976301d282e3b48e259f5f13d4b8bd to 515e343290448d2bbe5e75f88accea814be9215f
comment:50 Changed 6 weeks ago by rws
- Status changed from needs_work to needs_review
- Work issues circular import, doctest failures deleted
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.
comment:51 Changed 3 weeks ago by vbraun_spam
- Milestone changed from sage-6.3 to sage-6.4
comment:52 Changed 11 days ago by chapoton
- Status changed from needs_review to needs_work
one failing doctest, see patchbot report
comment:53 Changed 8 days ago by git
- Commit changed from 515e343290448d2bbe5e75f88accea814be9215f to 8e76828f9f5c988b0429230cd1050c6086fb730b
comment:54 Changed 8 days ago by rws
- Owner changed from olazo to rws
comment:55 Changed 8 days ago by rws
- Owner changed from rws to olazo
comment:56 Changed 8 days ago by rws
- Status changed from needs_work to needs_review
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:
You need to force it to evaluate:
2.- This doesn't work:
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.