Opened 10 years ago
Last modified 18 months ago
#6371 needs_work enhancement
Implement Riemann theta functions
Reported by:  ncalexan  Owned by:  cswiercz 

Priority:  minor  Milestone:  sage7.1 
Component:  numerical  Keywords:  riemann theta klein 
Cc:  cswiercz, fredrik.johannson, mstreng, jpflori  Merged in:  
Authors:  Nick Alexander, Chris Swierczewski  Reviewers:  Marco Streng, Bernard Deconinck 
Report Upstream:  N/A  Work issues:  
Branch:  u/chapoton/6371 (Commits)  Commit:  0342ce94e4dae0b15ba2509118f3ef120f4a69d1 
Dependencies:  Stopgaps: 
Description (last modified by )
In the theory of differential equations and Abelian varieties, Riemann theta functions and there relatives play an important role. Implement these in sage!
Attachments (9)
Change History (56)
Changed 10 years ago by
comment:1 Changed 10 years ago by
 Summary changed from implement Riemann theta functions to [with patch, needs work] implement Riemann theta functions
comment:2 Changed 10 years ago by
 Cc mstreng added
Marco, you might be interested in this work in progress code too.
comment:3 Changed 10 years ago by
Is double precision sufficient? Are the arguments large, integers or halfintegers? In either case, very fast evaluation of the incomplete gamma function should be possible.
comment:4 Changed 10 years ago by
To use this, type
sage: hg_sage.apply('http://trac.sagemath.org/sage_trac/attachment/ticket/6371/trac_6371riemanntheta.patch') sage: quit sage br sage: now you have the new code
comment:5 Changed 9 years ago by
 Owner changed from jkantor to cswiercz
 Report Upstream set to N/A
comment:6 followup: ↓ 7 Changed 9 years ago by
Over the past month or two I've done considerable work vetting out the bugs and errors in the above code. Within a week or so I hope to post a replacement patch. Hence, the change of owner. I'll keep everyone who's interested posted on my progress.
A word of warning, though: since I know a lot of people who are interested in my getting Riemann theta into Sage as soon as possible I had to strip Shimura phi, Siegel theta, and Klein P. I'm not qualified to review / bug fix that part of the code. I hope this is okay with the original authors and people who are interested in seeing this patch resolved. Perhaps we can move these implementations into a new patch once Riemann theta is implemented.
Again, I'll be posting a replacement patch sometime within the next two weeks or so.
comment:7 in reply to: ↑ 6 ; followup: ↓ 8 Changed 9 years ago by
A word of warning, though: since I know a lot of people who are interested in my getting Riemann theta into Sage as soon as possible I had to strip Shimura phi, Siegel theta, and Klein P. I'm not qualified to review / bug fix that part of the code. I hope this is okay with the original authors and people who are interested in seeing this patch resolved.
I am the original author, and I think this is a very reasonable decision! You should know that I use "Siegel theta" and "Riemann theta" interchangeably. Riemann theta often seems to mean just with characteristics (0, 0, ..., 0) and Siegel theta seems to be a Mathematicaism. Whatever we decide on is (probably) fine by me.
Perhaps we can move these implementations into a new patch once Riemann theta is implemented.
These bits are interesting for my research, but probably are not all that interesting more generally. Cut them for now.
Thanks for you efforts, Chris!
comment:8 in reply to: ↑ 7 Changed 9 years ago by
I am the original author, and I think this is a very reasonable decision! You should know that I use "Siegel theta" and "Riemann theta" interchangeably. Riemann theta often seems to mean just with characteristics (0, 0, ..., 0) and Siegel theta seems to be a Mathematicaism. Whatever we decide on is (probably) fine by me. Thanks for you efforts, Chris!
Huzzah! Thank _you_ for your help!
comment:9 Changed 8 years ago by
 Status changed from needs_work to needs_review
 Summary changed from [with patch, needs work] implement Riemann theta functions to [with patch, needs review] implement Riemann theta functions
Note that the second patch is meant to replace the first. Many many changes were made!
comment:10 Changed 8 years ago by
 Status changed from needs_review to needs_work
 Summary changed from [with patch, needs review] implement Riemann theta functions to Implement Riemann theta functions
Some clarifications and fine tuning needs to be made to the documentation.
comment:11 Changed 8 years ago by
 Status changed from needs_work to needs_review
comment:12 Changed 8 years ago by
 Description modified (diff)
comment:13 followup: ↓ 14 Changed 8 years ago by
Patchbot doesn't seem to get the message. New attempt: Apply trac_6371riemannthetaREPLACEMENT.patch, trac_6371riemannthetaREPLACEMENTPART2.patch, trac_6371_riemannthetaREPLACEMENTPART3.patch
comment:14 in reply to: ↑ 13 Changed 8 years ago by
Replying to mstreng:
Patchbot doesn't seem to get the message. New attempt: Apply trac_6371riemannthetaREPLACEMENT.patch, trac_6371riemannthetaREPLACEMENTPART2.patch, trac_6371_riemannthetaREPLACEMENTPART3.patch
I tried just deleting the first patch called "trac_6371riemanntheta.patch" but I don't have permissions to do so. I'm also unfamiliar with how to tell patchbot to ignore certain patches.
A manual importing of the three patches using sage hg import <the patch>
results in no issues.
comment:15 Changed 8 years ago by
 Description modified (diff)
comment:16 Changed 8 years ago by
 Status changed from needs_review to needs_work
doctest failures (details emailed to author)
comment:17 Changed 8 years ago by
 Description modified (diff)
 Status changed from needs_work to needs_review
Fixed issues with computing derivatives that resulted in doctest failures. Minor documentation fixes as well.
comment:18 Changed 8 years ago by
 Cc jpflori added
comment:19 Changed 8 years ago by
 Status changed from needs_review to needs_work
With further examples, I noticed that there were some calculation issues. Traced this back to mixture of notation under Heil's transformation. Rewriting good chunk of the code using only the notation from "Computing Riemann Theta Functions" by Deconinck et. al.
Changed 8 years ago by
Changed 8 years ago by
comment:20 Changed 8 years ago by
 Description modified (diff)
 Status changed from needs_work to needs_review
Changed 8 years ago by
comment:21 Changed 8 years ago by
 Description modified (diff)
comment:22 Changed 8 years ago by
 Description modified (diff)
comment:23 Changed 8 years ago by
 Status changed from needs_review to positive_review
comment:24 Changed 8 years ago by
 Reviewers set to Marco Streng, bern4rd (Bernard Deconinck??)
comment:25 Changed 8 years ago by
 Reviewers changed from Marco Streng, bern4rd (Bernard Deconinck??) to Marco Streng, Bernard Deconinck
comment:26 Changed 8 years ago by
 Milestone changed from sagefeature to sage4.8
comment:27 followup: ↓ 28 Changed 8 years ago by
 Status changed from positive_review to needs_work
I get a doctest failure in devel/sage/sage/all.py
comment:28 in reply to: ↑ 27 Changed 8 years ago by
Replying to jdemeyer:
I get a doctest failure in
devel/sage/sage/all.py
I see. The error is:
sage t "devel/sage6371/sage/all.py" ********************************************************************** File "/home/cswiercz/sage4.7.1/devel/sage6371/sage/all.py", line 17: sage: len(frames) Expected: 11 Got: 26 ********************************************************************** 1 items had failures: 1 of 8 in __main__.example_0 ***Test Failed*** 1 failures. For whitespace errors, see the file /home/cswiercz/.sage//tmp/all_20664.py [3.4 s]
However, I'm not qualified to make a fix since I don't really understand the purpose of this test. Does anyone on cc: know what's going on in this file and why my Riemann Theta code would affect it?
comment:29 followup: ↓ 30 Changed 8 years ago by
Chris: Have you read #10570? That explains what this test is about and why it is important. Your code evidently gets imported when Sage starts up, and does something potentially leaky.
comment:30 in reply to: ↑ 29 Changed 8 years ago by
Replying to was:
Chris: Have you read #10570? That explains what this test is about and why it is important. Your code evidently gets imported when Sage starts up, and does something potentially leaky.
Yes, I read this ticket but I'm not sure how to resolve the "potential leakiness".
One candidate for where this issue might be coming from is my use of a class constructor. I have some code in riemann_theta.py
in the following form:
def RiemannTheta_Constructor(foo, bar): class RiemannTheta(Function_RiemannTheta): def __init__(...): self.foo = foo self.bar = bar Function_RiemannTheta.__init__() return RiemannTheta(...) RiemannTheta = RiemannTheta_Constructor class Function_RiemannTheta(BuiltinFunction): ...
I needed to do things this way so I could parameterize RiemannTheta
by things such as directional derivative and Riemann matrix. Otherwise, things behave not as expected due to the way BuiltinFunction
behaves. (For example, two instances of RiemannTheta
would share the same Riemann matrix or derivatives.) With the above code structure I found that these issues don't arise and two instances of RiemannTheta
behaved independently as expected.
I'd hate to introduce memory leaks due to my lack of understanding of BuiltinFunction
. (Granted, the documentation for creating classes that inherit BuiltinFunction
is severely lacking as well.) However, I don't see how I'd start fixing the issue aside from increasing len(frames)
in the error message above from 11 to 26. Any suggested resources on how I could better perform what I'm doing in the class and function construction above? Again, the BuiltinFunction
documentation wasn't helpful beyond copying what preexisting inheritors in sage.functions
do.
comment:31 followup: ↓ 32 Changed 8 years ago by
I don't know much about BuiltinFunction
, but I suspect that it isn't meant for what you are trying to do. All the examples that I know of BuiltinFunction
are single fixed functions, independent of any parameters, such as cos, min, max, IndefiniteIntegral
, Gamma, dirac_delta, etc.
Your objects however depend on a parameter Omega, and are functions only of z. They are maps theta(Omega): CC^g > CC : z > theta(z)
. This will not be a "builtin function of Sage" for every Omega, so it does not sound to me like this should be a BuiltinFunction
object. Maybe the memory leak is caused by this.
The function theta: CC^g x H_g > CC : (z, Omega) > theta(z, Omega)
definitely make sense as a builtin function, but not a function theta(Omega) : CC^g > C
that depends on Omega. In other words, if you have a function for which Omega is not a parameter but an actual variable, then that would make a good builtin function.
ps. Could you make sure you break the lines in the patch at 79 characters?
comment:32 in reply to: ↑ 31 ; followup: ↓ 33 Changed 8 years ago by
Hello mstreng,
Replying to mstreng:
I don't know much about
BuiltinFunction
, but I suspect that it isn't meant for what you are trying to do. All the examples that I know ofBuiltinFunction
are single fixed functions, independent of any parameters, such as cos, min, max,IndefiniteIntegral
, Gamma, dirac_delta, etc.
It was my misunderstanding that BuiltinFunction
is necessary for doing symbolic manipulation. My only reason for using it is so I can compute derivatives, etc. If you know of an alternate way to enable symbolics then I'd love to hear about it!
Your objects however depend on a parameter Omega, and are functions only of z. They are maps
theta(Omega): CC^g > CC : z > theta(z)
. This will not be a "builtin function of Sage" for every Omega, so it does not sound to me like this should be aBuiltinFunction
object. Maybe the memory leak is caused by this.
I'll experiment with the code and see if this is an issue.
In practice, $\Omega$ is treated more as a parameter than an argument of the Riemann theta function. That is, $\Omega$ is usually fixed and $z$ varies. Several users of my code would like to see this behavior be reflected by having the Riemann theta function be "initialized" by this matrix.
However, if memory leaks are unavoidable in this context then I suppose I'll just have to make $\Omega$ behave like an argument no different from $z$.
The function
theta: CC^g x H_g > CC : (z, Omega) > theta(z, Omega)
definitely make sense as a builtin function, but not a functiontheta(Omega) : CC^g > C
that depends on Omega. In other words, if you have a function for which Omega is not a parameter but an actual variable, then that would make a good builtin function.
Makes sense.
ps. Could you make sure you break the lines in the patch at 79 characters?
Of course! (As an interminal emacs user I usually try to do this anyway.)
comment:33 in reply to: ↑ 32 ; followup: ↓ 34 Changed 8 years ago by
Hi Chris,
Replying to cswiercz:
If you know of an alternate way to enable symbolics then I'd love to hear about it!
I don't: I've never implemented any of them.
In practice, $\Omega$ is treated more as a parameter than an argument of the Riemann theta function. That is, $\Omega$ is usually fixed and $z$ varies.
:) Actually, when I use theta functions in my research, Omega is usually the variable and z is usually some fixed vector in with entries in QQ.
Anyway, what is the (programming technical) reason for having objects that depend on Omega? Is there some kind of caching involved? Some part of the algorithm that depends on Omega only and that you only want to do once?
Marco
comment:34 in reply to: ↑ 33 Changed 8 years ago by
Marco,
:) Actually, when I use theta functions in my research, Omega is usually the variable and z is usually some fixed vector in with entries in QQ.
Anyway, what is the (programming technical) reason for having objects that depend on Omega? Is there some kind of caching involved? Some part of the algorithm that depends on Omega only and that you only want to do once?
Interesting. I've only seen one other instance in which Omega was varied: it was in the context of solving particular boundary value problems.) If you don't mind, do you have a paper you can send me about your work? (cswiercz [at] gmail ]dot[ com)
I agree that there should be no distinction between the two parameters. From what I understood about BuiltinFunction}}, though, the purpose of distinguishing Omega as a parameter is to make it easy to send symbolic output. Since I initially made this decision, however, I've learned more about the way Sage ({{{BuiltinFuncion
?) handles inputs and determines if they're symbolic. A rewriting of the way arguments are sent to RiemannTheta
should be possible.
I'll see what I can do about restructuring the code. Thank you very much for your input and suggestions. I'll keep everyone posted on my (slow) progress.
comment:35 Changed 6 years ago by
 Milestone changed from sage5.11 to sage5.12
comment:36 Changed 6 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:37 Changed 5 years ago by
 Milestone changed from sage6.2 to sage6.3
comment:38 Changed 5 years ago by
 Milestone changed from sage6.3 to sage6.4
comment:39 Changed 5 years ago by
 Branch set to u/chapoton/6371
 Commit set to afacea6afb0568d0fd83ec7a33a4bf34b21efe2a
I made a git branch from the patches, but it does not work. I have not tried to find where problems are.
New commits:
403854b  Added Riemann theta functions to Sage.

ff08d10  Minor doctest issues resolved with riemann_theta.py.

3d0a00c  Fixed additional errors with docstrings and symmetric Riemann matrix testing.

afacea6  trac #6371 code cleanup

comment:40 Changed 5 years ago by
 Commit changed from afacea6afb0568d0fd83ec7a33a4bf34b21efe2a to 5caa1ecf9b0f2797d3bb3d6685cfaee474ae303b
Branch pushed to git repo; I updated commit sha1. New commits:
5caa1ec  trac #6371 ref links

comment:41 Changed 4 years ago by
 Milestone changed from sage6.4 to sage7.0
comment:42 Changed 4 years ago by
 Commit changed from 5caa1ecf9b0f2797d3bb3d6685cfaee474ae303b to 696b9945cbe4ec47bb9573e91c0c8170f720822a
comment:43 Changed 4 years ago by
 Description modified (diff)
 Milestone changed from sage7.0 to sage7.1
comment:44 Changed 3 years ago by
 Commit changed from 696b9945cbe4ec47bb9573e91c0c8170f720822a to d552509034fb8dd5d49963d4a413547d9002d717
Branch pushed to git repo; I updated commit sha1. New commits:
d552509  Merge branch 'u/chapoton/6371' into 7.3.b3

comment:45 Changed 3 years ago by
 Commit changed from d552509034fb8dd5d49963d4a413547d9002d717 to 4575a7b315836d0ec47f600d5b9bc5834b3e348b
Branch pushed to git repo; I updated commit sha1. New commits:
4575a7b  Merge branch 'u/chapoton/6371' in 7.3.b7

comment:46 Changed 2 years ago by
 Commit changed from 4575a7b315836d0ec47f600d5b9bc5834b3e348b to 5fd3c600860bc27211d7f0a5ebec2fe6c50a7abf
Branch pushed to git repo; I updated commit sha1. New commits:
5fd3c60  Merge branch 'u/chapoton/6371' in 8.0.b9

comment:47 Changed 18 months ago by
 Commit changed from 5fd3c600860bc27211d7f0a5ebec2fe6c50a7abf to 0342ce94e4dae0b15ba2509118f3ef120f4a69d1
Branch pushed to git repo; I updated commit sha1. New commits:
0342ce9  Merge branch 'u/chapoton/6371' in 8.2.b8

I want to make sure this code doesn't get lost. It's very much [needs work]  I don't think all tests are tagged
optional
andlong
correctly. I'm not certain that the output is correct to very high accuracy, either  magma and mathematica disagree, and maple is too slow to evaluate to high precision.Fredrik, somewhere in here there are lots of calls to the incomplete gamma function. I used low precision as much as possible as you will see, because I don't really need high precision, I just need large outputs.
Chris, this applies against 4.0.2.alpha1 at least. Documentation and testing appreciated!