-
-
Notifications
You must be signed in to change notification settings - Fork 512
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
Sage is missing the lambert_w function #11888
Comments
Attachment: trac_11888.patch.gz add lambert_w symbolic function |
Changed keywords from lambert_w symbolics conversion maxima to lambert_w symbolics conversion maxima sd35.5 |
Author: Benjamin Jones |
comment:2
Preliminary patch needs review. The function has been added using the template developed as part of #11143. The issue described in the description is addressed in one of the doctests. |
apply to $SAGE_ROOT/devel/sage |
comment:3
Attachment: trac_11888-doctests.patch.gz Running |
This comment has been minimized.
This comment has been minimized.
comment:4
All tests pass. |
comment:6
Thanks for the fixes, kini. I've run |
comment:7
I don't see any obvious problems, but the random expression usually doesn't change much with these new functions and this one is really different. It's also spread across many lines, and I'm not sure if this is appropriate (just in this one case, of course). |
comment:8
I spread it across lines because 1) I was trying to keep within the recommended PEP 8 guidelines for line length, and 2) because of this
What is inappropriate about adding line breaks? As for the length of the expression, it seems to be a fluke. With the patches applied, starting with random seeds other than |
comment:9
I agree, it looks like a fluke that the expression grows so large. I did some testing of |
comment:10
I strongly recommend implementing the general version of the Lambert W function (taking a branch parameter). |
comment:11
Can you be more specific? (Is this standard with other multivalued functions in Sage?) Maybe this could be a separate ticket, unless the change was really easy. |
comment:12
The change should be simple. mpmath implements the a branch |
comment:13
Sweet, I didn't realize it was that quick. I love doing Sage development on that train :) There is also free wifi at Logan, I believe. |
comment:14
Ping. I'd love to review this but sounds like Fredrik's point is good and if it's pretty easy for you to add that, we might as well. |
comment:15
Yes, it should be easy; just add an optional branch parameter, lambertw(z, branch=0). Another suggestion is to use scipy.special.lambertw for evaluation over RDF and CDF. The SciPy implementation is a Cython translation of the double precision version in mpmath; it supports all branches and has excellent numerical stability, and runs quite a bit faster.
|
comment:16
Nice; I wonder if there are more places we are beginning to default to mpmath where SciPy could be useful for the double fields. |
Work Issues: add second parameter, RDF/CDF stuff |
Reviewer: Keshav Kini, Karl-Dieter Crisman, Fredrik Johansson |
comment:17
Thanks for the ping. I'm still here (and I have a patch pretty much ready to go) I just got buried under teaching. I'll try to upload a patch this evening. |
comment:51
I can make a quick spelling fix patch, but I'll wait and see if anyone else has changes to suggest. |
comment:52
After some real-world discussions, I'd prefer to avoid falling into Python ints for (some of) the special values:
Mysteriously enough, instrumenting it reveals that |
comment:53
It looks like whatever happens after |
comment:54
One solution is to change the return statements in these special cases where the automatic simplification returns an integer to just explicitly return How does that sound? |
This comment has been minimized.
This comment has been minimized.
comment:55
New patch is ready for review. I hope this can be the final revision! Patchbot: apply attachment: trac_11888_v8.patch to $SAGE_ROOT/devel/sage |
comment:56
Looking at it now. |
fixed spelling / grammer mistakes, returned parent(Integer(...)) for special values |
comment:57
Attachment: trac_11888_v8.patch.gz Okay, this looks good. Two copyedits, an extra doc describing the behaviours of the derivative function, some tests making sure we can't differentiate with respect to the branch number, and the addition of lambert_W(-pi/2) = pi/2*I as a special value. I give positive review to the preexisting parts of v8; if the new bits of v9 look okay I think we're good to go. |
Attachment: trac_11888_v9.patch.gz post-review version of lambertw sf |
comment:58
New changes look good; good catch about the derivative w.r.t. branch. All relavent tests pass for me on sage-5.0. I would say this is ready to go in! Thanks for the very thorough review, Doug. |
Changed reviewer from Keshav Kini, Karl-Dieter Crisman, Fredrik Johansson, Burcin Erocal to Keshav Kini, Karl-Dieter Crisman, Fredrik Johansson, Burcin Erocal, Douglas McNeil |
comment:60
+1. Tx for the work! |
Changed reviewer from Keshav Kini, Karl-Dieter Crisman, Fredrik Johansson, Burcin Erocal, Douglas McNeil to Keshav Kini, Karl-Dieter Crisman, Fredrik Johansson, Burcin Erocal, Douglas McNeil, William Stein |
comment:61
This patch is awesome! It's also a great example of how to make a well-documented new symbolic function that illustrates many issues. Here are a few trivial nitpicks:
(check for similar instances throughout).
Here's a good example of what an exception string should look like (built into python):
|
comment:62
Updated version taking into account comments of was. |
minor edits |
comment:64
Attachment: trac_11888_v10.patch.gz |
Merged: sage-5.1.beta4 |
Maxima returns solutions to some exponential equations in terms of the
lambert_w
function. Sage is missing a conversion for this function:mpmath
can evaluate thelambert_w
function, so it should be easy to add a new symbolic function to Sage that will fix this issue.Apply:
$SAGE_ROOT/devel/sage
CC: @kcrisman @sagetrac-ktkohl
Component: symbolics
Keywords: lambert_w symbolics conversion maxima sd35.5 sd40.5
Author: Benjamin Jones
Reviewer: Keshav Kini, Karl-Dieter Crisman, Fredrik Johansson, Burcin Erocal, Douglas McNeil, William Stein
Merged: sage-5.1.beta4
Issue created by migration from https://trac.sagemath.org/ticket/11888
The text was updated successfully, but these errors were encountered: