-
-
Notifications
You must be signed in to change notification settings - Fork 487
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 bessel_J symbolic #4102
Comments
Changed keywords from none to jason |
Changed keywords from jason to none |
comment:5
This ticket description was too broad. We have lots of tickets on fixing symbolic functions, see symbolics/functions for an overview. See #3426 and #4230 for other tickets related to bessel functions. It would make sense to fix all these together, with a base class that handles the common properties of bessel functions (differentiation), and subclasses that implement numerical evaluation, etc. for each type. |
This comment has been minimized.
This comment has been minimized.
comment:7
See also #10636, which I somehow never saw before. This sage-support thread yields an interesting related error:
Note that we apparently aren't yet converting Maxima's Bessel properly to 'our' uppercase version because of this. |
comment:8
This wouldn't be hard to implement using #11143 as a template, but what to do about names? I guess new names for the symbolic Bessel functions should be chosen and deprecation notices added to the existing What new names do people like in the interim?
??? |
comment:9
Replying to @benjaminfjones:
What is wrong with taking over Thanks for looking into this. |
comment:10
I concur.
One could use that as the "symbolic" class one and then do the usual
Agreed! |
Author: Benjamin Jones |
comment:12
I thought I'd post some work in progress for all the Bessel function fans in the crowd. There is still work to be done, e.g.
but at this point the patch applies cleanly to 5.4.1 and all the doctests in If you are interested, have a look at the doctests included with the Thanks! |
work in progress making Bessel functions symbolic |
comment:13
Attachment: trac_symbolic_bessel.metaclass.2.patch.gz Nice! A few comments of the type you solicited:
|
comment:14
Thanks for the patch. Bessel functions are symbolic with so few lines of code. Amazing. :) I like the metaclass idea. That never occured to me before as an option for functions with parameters, like the order here. I have a few questions:
I am also concerned about blowing up the symbolic function registry with these instances. Note that we keep an entry in a C++ list with a pointer to all the possible custom methods, and a Python dictionary with a function -> Pynac id mapping for each subclass of BTW, I suggest moving this code to a new file |
comment:15
Replying to @kcrisman: Thanks for the comments. I finally found some time to get back to this ticket :)
That was intentional, I was trying not to shadow the builtin name.
I think
Good question, I haven't tested it out. I think I'm going to rewrite most of the code anyway so I'll keep this in mind.
???
Good point, I'll make sure to doctest the exceptions.
I don't know about this. I had this in the case of the single parameter functions like
Ideally all of them. Some will need to be modified because of the change in numerical back-end.
|
comment:16
Replying to @burcin: Thanks for looking at it!
The advantage I had in mind was just code reuse. In hindsight though, this approach makes the code more complicated and less maintainable that it needs to be. I'm going to refactor it into four generic functions in
Good point..
Also a good point. This occurred to me, but I didn't think through the consequences very much. I can see it being a problem if a user can inadvertently create a very large number of symbolic functions at runtime by doing something innocuous like:
Good idea. New patch coming soon... |
comment:17
Such as this and this. Just for forward-compatibility (instead of the percent business). Problem is that when I looked for ways to get around braces "naturally" occurring in LaTeX, there weren't necessarily a lot of them. (Ways, that is.) |
comment:18
Replying to @kcrisman:
Oh, right. I just didn't see what in the code you were referring to. I see now. Anyway, to one of your earlier questions, with the new code I'm about to post the following conversions to and from Maxima work great:
|
Attachment: trac_symbolic_bessel_v2.patch.gz more work in progress |
comment:19
Just as an FYI, this ask.sagemath question wants lots and lots of precision on Bessel Y. Which I think will be provided here via mpmath - just pointing out we should doctest it. |
comment:21
Yet more work in progress, added lots of doctests, in particular to show that problems in #4230 and #3426 are fixed by this patch. One feature this patch adds is the ability to solve Added brief mathematical exposition on the module docstring as well as some usage EXAMPLES. I think the patch is more or less ready for initial review. All tests withing the new file |
more work in progress, v3 |
comment:22
Attachment: trac_symbolic_bessel_v3.patch.gz Dumb comments since I don't have time for proper review - and more importantly, there are no obvious horrible things (though I haven't gone in depth with branches yet). If all this really works and there are no typos, I think this will be a VERY nice addition.
|
Reviewer: Karl-Dieter Crisman |
comment:76
I uploaded a new patch to fix doctest failures caused by #9880. |
This comment has been minimized.
This comment has been minimized.
comment:77
Looks good and passes tests. |
comment:78
On
|
comment:79
Attachment: trac_4102-bessel_doctest_fixes2.patch.gz New patch, adding a tolerance for the integral which is higher than the maximum error given by GSL. |
This comment has been minimized.
This comment has been minimized.
comment:80
Thanks! |
Merged: sage-5.11.rc0 |
comment:82
This is wrong:
|
Attachment: trac4102_diff.patch.gz |
comment:83
New patch fixes this issue. Can this be merged in 5.11? |
comment:84
Replying to @eviatarbach:
No, you should make a follow-up ticket for this. |
comment:85
Oh okay, then can this ticket be removed from 5.11? I'm just worried about having mathematically incorrect answers in the release. |
comment:86
Replying to @eviatarbach:
Sorry, I really meant: perhaps yes it can be in sage-5.11, but in any case there needs to be a follow-up ticket (make it milestone: sage-5.11 and priority: blocker) and the new patch needs to be reviewed. |
comment:87
Also, add a doctest for the |
comment:88
Okay, thank you. This is now #15019. |
The motivation for this is
The problem is that special functions, or at least
bessel_J
, can't currently be partially evaluated--that is, called with aSymbolicExpression
as an argument. The model of good behavior ispolylog
, for which the above method produces a perfectly nice plotSee discussion at http://groups.google.com/group/sage-support/browse_thread/thread/1b985b080ba2369e/7dee9eed953857f5#7dee9eed953857f5
Release manager:
Apply:
Depends on #9880
CC: @jasongrout @kcrisman @benjaminfjones @sagetrac-dsm @burcin @eviatarbach
Component: calculus
Keywords: sd48
Author: Benjamin Jones, Eviatar Bach, Volker Braun
Reviewer: Karl-Dieter Crisman, Burcin Erocal
Merged: sage-5.11.rc0
Issue created by migration from https://trac.sagemath.org/ticket/4102
The text was updated successfully, but these errors were encountered: