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: sage-7.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 chapoton)

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)

trac_6371-riemann-theta.patch (112.2 KB) - added by ncalexan 10 years ago.
trac_6371-riemann-theta-REPLACEMENT.patch (70.7 KB) - added by cswiercz 8 years ago.
Replacement for first patch by ncalexan
trac_6371-riemann-theta-REPLACEMENT-PART2.patch (9.4 KB) - added by cswiercz 8 years ago.
Part 2 of the replacement patch. Includes documentation fixes.
trac_6371_riemann-theta-REPLACEMENT-PART3.patch (827 bytes) - added by cswiercz 8 years ago.
Minor last-minute bug.
trac_6371-riemann-theta-REPLACEMENT-PART4.patch (4.7 KB) - added by cswiercz 8 years ago.
More documentation fixes.
trac_6371-riemann-theta-REPLACEMENT-PART5.patch (5.5 KB) - added by cswiercz 8 years ago.
Fixed derivative calculation errors that resulted in incorrect doctests.
trac-6371.patch (49.0 KB) - added by cswiercz 8 years ago.
trac-6371-part2.patch (2.2 KB) - added by cswiercz 8 years ago.
trac-6371-part3.patch (4.8 KB) - added by cswiercz 8 years ago.

Download all attachments as: .zip

Change History (56)

Changed 10 years ago by ncalexan

comment:1 Changed 10 years ago by ncalexan

  • Summary changed from implement Riemann theta functions to [with patch, needs work] implement Riemann theta functions

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 and long 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!

comment:2 Changed 10 years ago by ncalexan

  • Cc mstreng added

Marco, you might be interested in this work in progress code too.

comment:3 Changed 10 years ago by fredrik.johansson

Is double precision sufficient? Are the arguments large, integers or half-integers? In either case, very fast evaluation of the incomplete gamma function should be possible.

comment:4 Changed 10 years ago by was

To use this, type

sage: hg_sage.apply('http://trac.sagemath.org/sage_trac/attachment/ticket/6371/trac_6371-riemann-theta.patch')
sage: quit

sage -br

sage: now you have the new code

comment:5 Changed 9 years ago by cswiercz

  • Owner changed from jkantor to cswiercz
  • Report Upstream set to N/A

comment:6 follow-up: Changed 9 years ago by cswiercz

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 ; follow-up: Changed 9 years ago by ncalexan

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 Mathematica-ism. 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 cswiercz

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 Mathematica-ism. Whatever we decide on is (probably) fine by me. Thanks for you efforts, Chris!

Huzzah! Thank _you_ for your help!

Changed 8 years ago by cswiercz

Replacement for first patch by ncalexan

comment:9 Changed 8 years ago by cswiercz

  • Authors changed from Nick Alexander to Nick Alexander, cswiercz
  • 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 cswiercz

  • 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.

Changed 8 years ago by cswiercz

Part 2 of the replacement patch. Includes documentation fixes.

comment:11 Changed 8 years ago by cswiercz

  • Status changed from needs_work to needs_review

Changed 8 years ago by cswiercz

Minor last-minute bug.

comment:13 follow-up: Changed 8 years ago by mstreng

Patchbot doesn't seem to get the message. New attempt: Apply trac_6371-riemann-theta-REPLACEMENT.patch, trac_6371-riemann-theta-REPLACEMENT-PART2.patch, trac_6371_riemann-theta-REPLACEMENT-PART3.patch

comment:14 in reply to: ↑ 13 Changed 8 years ago by cswiercz

Replying to mstreng:

Patchbot doesn't seem to get the message. New attempt: Apply trac_6371-riemann-theta-REPLACEMENT.patch, trac_6371-riemann-theta-REPLACEMENT-PART2.patch, trac_6371_riemann-theta-REPLACEMENT-PART3.patch

I tried just deleting the first patch called "trac_6371-riemann-theta.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.

Changed 8 years ago by cswiercz

More documentation fixes.

comment:15 Changed 8 years ago by cswiercz

  • Description modified (diff)

comment:16 Changed 8 years ago by mstreng

  • Status changed from needs_review to needs_work

doctest failures (details emailed to author)

Changed 8 years ago by cswiercz

Fixed derivative calculation errors that resulted in incorrect doctests.

comment:17 Changed 8 years ago by cswiercz

  • 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 jpflori

  • Cc jpflori added

comment:19 Changed 8 years ago by cswiercz

  • 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 cswiercz

Changed 8 years ago by cswiercz

comment:20 Changed 8 years ago by cswiercz

  • Description modified (diff)
  • Status changed from needs_work to needs_review

Changed 8 years ago by cswiercz

comment:21 Changed 8 years ago by cswiercz

  • Description modified (diff)

comment:22 Changed 8 years ago by cswiercz

  • Description modified (diff)

comment:23 Changed 8 years ago by bern4rd

  • Status changed from needs_review to positive_review

comment:24 Changed 8 years ago by mstreng

  • Authors changed from Nick Alexander, cswiercz to Nick Alexander, Chris Swierczewski
  • Reviewers set to Marco Streng, bern4rd (Bernard Deconinck??)

comment:25 Changed 8 years ago by cswiercz

  • Reviewers changed from Marco Streng, bern4rd (Bernard Deconinck??) to Marco Streng, Bernard Deconinck

comment:26 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-feature to sage-4.8

comment:27 follow-up: Changed 8 years ago by jdemeyer

  • 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 cswiercz

Replying to jdemeyer:

I get a doctest failure in devel/sage/sage/all.py

I see. The error is:

sage -t  "devel/sage-6371/sage/all.py"                      
**********************************************************************
File "/home/cswiercz/sage-4.7.1/devel/sage-6371/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 follow-up: Changed 8 years ago by 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.

comment:30 in reply to: ↑ 29 Changed 8 years ago by cswiercz

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 follow-up: Changed 8 years ago by 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 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 "built-in 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 built-in 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 built-in function.

ps. Could you make sure you break the lines in the patch at 79 characters?

comment:32 in reply to: ↑ 31 ; follow-up: Changed 8 years ago by cswiercz

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 of BuiltinFunction 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 "built-in 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.

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 built-in 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 built-in function.

Makes sense.

ps. Could you make sure you break the lines in the patch at 79 characters?

Of course! (As an in-terminal emacs user I usually try to do this anyway.)

comment:33 in reply to: ↑ 32 ; follow-up: Changed 8 years ago by mstreng

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 cswiercz

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 jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:36 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:37 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:38 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:39 Changed 5 years ago by chapoton

  • 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:

403854bAdded Riemann theta functions to Sage.
ff08d10Minor doctest issues resolved with riemann_theta.py.
3d0a00cFixed additional errors with docstrings and symmetric Riemann matrix testing.
afacea6trac #6371 code cleanup

comment:40 Changed 5 years ago by git

  • Commit changed from afacea6afb0568d0fd83ec7a33a4bf34b21efe2a to 5caa1ecf9b0f2797d3bb3d6685cfaee474ae303b

Branch pushed to git repo; I updated commit sha1. New commits:

5caa1ectrac #6371 ref links

comment:41 Changed 4 years ago by chapoton

  • Milestone changed from sage-6.4 to sage-7.0

comment:42 Changed 4 years ago by git

  • Commit changed from 5caa1ecf9b0f2797d3bb3d6685cfaee474ae303b to 696b9945cbe4ec47bb9573e91c0c8170f720822a

Branch pushed to git repo; I updated commit sha1. New commits:

72a17d9Merge branch 'u/chapoton/6371' into 7.1.b3
5e44969trac #6371 partial correction, tolerance added
696b994trac #6371 details

comment:43 Changed 4 years ago by chapoton

  • Description modified (diff)
  • Milestone changed from sage-7.0 to sage-7.1

comment:44 Changed 3 years ago by git

  • Commit changed from 696b9945cbe4ec47bb9573e91c0c8170f720822a to d552509034fb8dd5d49963d4a413547d9002d717

Branch pushed to git repo; I updated commit sha1. New commits:

d552509Merge branch 'u/chapoton/6371' into 7.3.b3

comment:45 Changed 3 years ago by git

  • Commit changed from d552509034fb8dd5d49963d4a413547d9002d717 to 4575a7b315836d0ec47f600d5b9bc5834b3e348b

Branch pushed to git repo; I updated commit sha1. New commits:

4575a7bMerge branch 'u/chapoton/6371' in 7.3.b7

comment:46 Changed 2 years ago by git

  • Commit changed from 4575a7b315836d0ec47f600d5b9bc5834b3e348b to 5fd3c600860bc27211d7f0a5ebec2fe6c50a7abf

Branch pushed to git repo; I updated commit sha1. New commits:

5fd3c60Merge branch 'u/chapoton/6371' in 8.0.b9

comment:47 Changed 18 months ago by git

  • Commit changed from 5fd3c600860bc27211d7f0a5ebec2fe6c50a7abf to 0342ce94e4dae0b15ba2509118f3ef120f4a69d1

Branch pushed to git repo; I updated commit sha1. New commits:

0342ce9Merge branch 'u/chapoton/6371' in 8.2.b8
Note: See TracTickets for help on using tickets.