Opened 3 years ago
Closed 2 years ago
#14996 closed enhancement (fixed)
Improve support for Jacobi elliptic functions
Reported by:  eviatarbach  Owned by:  

Priority:  major  Milestone:  sage6.3 
Component:  symbolics  Keywords:  
Cc:  burcin, kcrisman, benjaminfjones, fredrik.johansson  Merged in:  
Authors:  Eviatar Bach  Reviewers:  Ralf Stephan, Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  6df0d14 (Commits)  Commit:  6df0d14c474715ac2565c2a9abea51022fc945dd 
Dependencies:  Stopgaps: 
Description
Currently, all evaluation (numeric and symbolic) of the Jacobi elliptic functions is done through Maxima. No derivatives or arbitraryprecision numeric evaluation are defined. Worse still, the Maxima numerical evaluation is wrong for some of the inverse Jacobi functions; see https://sourceforge.net/p/maxima/bugs/2615/.
Attachments (4)
Change History (25)
Changed 3 years ago by
comment:1 Changed 3 years ago by
 Status changed from new to needs_review
comment:2 Changed 3 years ago by
 Cc burcin added
comment:3 Changed 3 years ago by
 Cc kcrisman benjaminfjones added
comment:4 Changed 3 years ago by
 Cc fredrik.johansson added
I hope you don't mind me CC'ing you.
In this patch I add the functions inverse_jacobi_f
and jacobi_amplitude_f
which use mpmath. Do you think these could be added to a future version? I know I'm not smart about increasing the context precision for successive computations but it seems to work.
comment:5 Changed 3 years ago by
Sure, they could be added to a future version. I never even tried to implement them because it seemed like too much work to figure out all the case distinctions and writing test code for branch cuts. You seem to restrict the code to real arguments, which makes things easier and probably makes a good enough start.
comment:6 Changed 3 years ago by
Ah, good. Should I submit a pull request on GitHub? or something?
The reason I used real numbers for the inverse Jacobi is because it was a pain to get continuity otherwise. For the amplitude function, it's defined in the DLMF for real numbers (http://dlmf.nist.gov/22.16).
comment:7 Changed 3 years ago by
Sure, you're very welcome to supply a patch if you have time.
Better put this code in Sage now though, and then perhaps replace it with calls to mpmath in the future (can't guarantee when the next release of mpmath will happen, regrettably).
comment:8 Changed 3 years ago by
Thanks, I will do so when I have time.
New patch fixes two bugs.
Changed 3 years ago by
comment:9 Changed 3 years ago by
New patch rebases to Sage 5.11, gets coverage to 100%, adds tons of new tests, and makes all Python int
s in simplifications and derivatives Sage integers.
Please don't be worried about the length of the patch, much of it derives from identities and tests. Since there are 12 Jacobi elliptic functions and all their inverses, this adds up to a lot.
Patchbot apply trac14996_3.patch
Changed 3 years ago by
comment:10 Changed 3 years ago by
I did some tests, and the doctest for plot/line.py failed (it tries to plot a Jacobi elliptic function at line 413 and this goes awry in trying to convert a complex number which is very close into a real number to a real number).
A small issue is that perhaps jacobi
should complain if one passes to it as the first argument a string which is not of the form 'pq' with p, q one of s, c, n, d. Moreover, if the argument is 'pp' then it could perhaps use the convention used in the documentation and return 1?
I'm new to sage, apologies for the inanity of my comments.
comment:11 Changed 3 years ago by
No, they're not inane at all! Thanks for the comments, they bring up important issues to fix.
You're right about the doctest and the need to raise an error; the Jacobi
class raises an error but I forgot to add it for the jacobi
function. Fix coming up shortly.
As for the pp=1
convention, it could be done but I don't see how it would be useful. In all sources I've seen they're called the "12 Jacobi functions". Also, it might give users the impression that Sage is actually calculating the ratios of the functions and simplifying. To remain consistent with the other functions we would also have to add the aliases jacobi_nn
, jacobi_ss
, etc., which amounts to just adding four constants that equal 1 to the global namespace.
comment:12 Changed 3 years ago by
New patch fixes doctests, adds errors when an invalid Jacobi function name is given, and updates LaTeX conversions to follow the convention of using  as the parameter delimiter.
Patchbot apply trac14996_4.patch
Changed 3 years ago by
comment:13 Changed 3 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:14 Changed 3 years ago by
 Branch set to u/rws/ticket/14996
 Created changed from 08/03/13 04:09:33 to 08/03/13 04:09:33
 Modified changed from 01/30/14 21:20:52 to 01/30/14 21:20:52
comment:15 Changed 3 years ago by
 Commit set to a6ab7a4bde8b2ebe8e6b51a4fec7ff8828e033e3
 Reviewers set to Ralf Stephan
 Status changed from needs_review to positive_review
I think this is fine. It tests long OK in functions/ and builds all docs. The docs look good too. The branch is on top of 6.2.beta2.
Author copied from patch header.
New commits:
a6ab7a4  Trac #14996: Rewriting Jacobi elliptic function code, new numeric evaluation, Jacobi amplitude function

comment:16 Changed 3 years ago by
comment:17 Changed 3 years ago by
Fails with
sage t src/sage/functions/jacobi.py ********************************************************************** File "src/sage/functions/jacobi.py", line 210, in sage.functions.jacobi.Jacobi._eval_ Failed example: almosteq(n(jacobi_nd(8, 0, hold=True)), n(jacobi_nd(8, 0))) Exception raised: Traceback (most recent call last): File "/home/release/Sage/local/lib/python2.7/sitepackages/sage/doctest/forker.py", line 480, in _run self.execute(example, compiled, test.globs) File "/home/release/Sage/local/lib/python2.7/sitepackages/sage/doctest/forker.py", line 839, in execute exec compiled in globs File "<doctest sage.functions.jacobi.Jacobi._eval_[1]>", line 1, in <module> almosteq(n(jacobi_nd(Integer(8), Integer(0), hold=True)), n(jacobi_nd(Integer(8), Integer(0)))) File "/home/release/Sage/local/lib/python2.7/sitepackages/sage/misc/functional.py", line 1413, in numerical_approx return x._numerical_approx(prec, algorithm=algorithm) File "expression.pyx", line 4704, in sage.symbolic.expression.Expression._numerical_approx (sage/symbolic/expression.cpp:23432) File "expression.pyx", line 1036, in sage.symbolic.expression.Expression._convert (sage/symbolic/expression.cpp:7216) TypeError: _evalf_() got an unexpected keyword argument 'algorithm'
Possibly due to the pynac update which I merged first #15198
comment:18 Changed 3 years ago by
Presumably due to the simultaneouslymerged and related #14778, but same difference  I am sure Eviatar can update for this very easily.
comment:19 Changed 3 years ago by
 Branch changed from u/rws/ticket/14996 to u/tscrim/14996
 Commit changed from a6ab7a4bde8b2ebe8e6b51a4fec7ff8828e033e3 to 6df0d14c474715ac2565c2a9abea51022fc945dd
 Reviewers changed from Ralf Stephan to Ralf Stephan, Travis Scrimshaw
I've fixed the failing doctests and doc did not build, so I fixed that as well. I also did some formatting cleanup of the doc (while I was reading it over trying to find the docbuild issues).
New commits:
db1980b  Merge branch 'u/rws/ticket/14996' of trac.sagemath.org:sage into u/tscrim/14996

6df0d14  Fixed doctests and documentation. Additional doc cleanup.

comment:20 Changed 2 years ago by
 Milestone changed from sage6.2 to sage6.3
comment:21 Changed 2 years ago by
 Branch changed from u/tscrim/14996 to 6df0d14c474715ac2565c2a9abea51022fc945dd
 Resolution set to fixed
 Status changed from positive_review to closed
The patch rewrites the Jacobi functions as standard symbolic functions. I wrote a new numerical routine for the inverse Jacobi functions, since it didn't seem to be available elsewhere. I've also added the Jacobi amplitude function.