-
-
Notifications
You must be signed in to change notification settings - Fork 565
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
Make Airy functions symbolic #12455
Comments
This comment has been minimized.
This comment has been minimized.
comment:4
Attachment: trac_12455-symbolic_airy.patch.gz 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} And also whether the latex representation should be capitalized or not. I chose the third scheme, and capitalized typesetting. |
comment:6
Good start. A few responses.
Thanks for getting us a start on this! That's great. |
comment:7
mpmath has the derivatives (as well as integrals) -- just use the optional 'derivative' parameter. |
comment:8
Ok, I didn't realize that was what that parameter was about. Though in retrospect it seems obvious! |
comment:9
On second thought, I think it would be better to use the airy equation to calculate derivatives or order higher than 1. Like
which is very likey to be the way mpmath calculates higher order derivatives. Integrals however, would be returned as:
what do you think? |
comment:10
Attachment: trac_12455-symbolic_airy2.patch.gz 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:
Is that important? |
comment:11
The second part isn't, as you are implicitly testing these 'hidden' functions. By the way,
should be
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
would work when you run tests, assuming you didn't import 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
It looks like they do:
Why shouldn't they? I'll take care of the |
comment:13
I'm sorry, that should be:
(the command was sage -t) |
comment:14
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:
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. |
comment:15
Attachment: trac_12455-symbolic_airy3.patch.gz I just added a new patch, now with 100 % test coverage, but I still get this message:
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). |
Author: Oscar Gerardo Lazo Arjona |
comment:78
x should be z in the differential equation |
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. |
comment:80
More nitpicking, not necessarily for this ticket:
|
comment:81
Replying to @mezzarobba:
Scratch that, I misread the code. |
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:83
One last thing: IMO |
comment:84
Replying to @mezzarobba:
Strange typo.
A quick look at old and new code shows 1.
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. |
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
|
comment:86
Replying to @rwst:
Both done (now rather than later to avoid breaking the interface).
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! |
Changed reviewer from Eviatar Bach, Karl-Dieter Crisman, Burcin Erocal, Ralf Stephan to Eviatar Bach, Karl-Dieter Crisman, Burcin Erocal, Ralf Stephan, Jeroen Demeyer, Marc Mezzarobba |
This comment has been minimized.
This comment has been minimized.
comment:87
Is fine and passes tests in |
Changed branch from public/ticket/12455 to |
Changed author from Oscar Gerardo Lazo Arjona, Benjamin Jones, D. S. McNeil, Eviatar Bach, Ralf Stephan to Oscar Gerardo Lazo Arjona, Benjamin Jones, Douglas McNeil, Eviatar Bach, Ralf Stephan |
Changed commit from |
As discussed in sage-support.
Currently sage can evaluate airy functions numerically:
but it doesn't work symbolically
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
The text was updated successfully, but these errors were encountered: