#3934 closed enhancement (fixed)
[with patches; positive review] Eta product modular functions
Reported by: | David Loeffler | Owned by: | David Loeffler |
---|---|---|---|
Priority: | minor | Milestone: | sage-3.2 |
Component: | modular forms | Keywords: | |
Cc: | Merged in: | 3.2.alpha1 | |
Authors: | David Loeffler | Reviewers: | John Cremona |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description
At the Heilbronn Institute workshop "Computations with Modular Forms", Ken McMurdy requested code for handling eta products on X_0(N). Theory background and wish-list here: http://maths.pratum.net/CMF/resources/McMurdyTalk.pdf.
Attachments (4)
Change History (20)
comment:1 Changed 14 years ago by
Milestone: | → sage-feature |
---|---|
Status: | new → assigned |
Changed 14 years ago by
Attachment: | 10292.patch added |
---|
comment:2 Changed 14 years ago by
Summary: | Eta product modular functions → [with patch, needs review] Eta product modular functions |
---|
comment:3 Changed 14 years ago by
Summary: | [with patch, needs review] Eta product modular functions → [with patch, with partial review] Eta product modular functions |
---|
Patch applies fine to 3.1.1. Doctests for the new file pass.
Problem: doctesting all in sage/modular, some strange errors appear in modular/abvar to do with latex. They went away when I deleted the line "... import latex" in the new file, which is not actually used anyway.
In the constructor, in checking Ligozat, you loop over divisors of N. Would it not be faster to loop over the keys in the dict? There is then no need to factor N, or to look at (however briefly) the divisors which are not in the dict.
There is something similar at line 146. Instead of
eta_n = max([ (n/d).floor() for d in divisors(self.level()) if self.r(d) != 0])
you could write
eta_n = max([ (n/d).floor() for d in self.r().keys()])
If you really wanted to have the full list of divisors of the level around, you could compute it and cache the result in the constructor.
In the function r(d) you can just say
return self._rdict.get(d,0)
since the get function on a dict returns the value if the key is there or a default value (here 0) if not.
That's all I have time for now: I got as far as line 235 (def basis_eta_products).
comment:4 Changed 14 years ago by
Summary: | [with patch, with partial review] Eta product modular functions → [with patch, with partial review, needs further work] Eta product modular functions |
---|
Thanks very much for the comments. Since that first version was written I have realised a much better way of fitting the code into Sage's object hierarchy; I will combine that with your suggestions and submit a new version.
Changed 14 years ago by
Attachment: | 10293.patch added |
---|
comment:5 follow-up: 6 Changed 14 years ago by
Summary: | [with patch, with partial review, needs further work] Eta product modular functions → [with patch, with partial review] Eta product modular functions |
---|
OK, here's a second attempt (patch to be installed on top of the first attempt). I've refactored it extensively, creating a new class EtaGroup which is the parent of eta product objects; changed it so it doesn't use divisors() unless it really has to; and added a new module-level function eta_poly_relations which finds polynomial relations between two eta products.
comment:6 Changed 14 years ago by
Replying to davidloeffler:
OK, here's a second attempt (patch to be installed on top of the first attempt). I've refactored it extensively, creating a new class EtaGroup which is the parent of eta product objects; changed it so it doesn't use divisors() unless it really has to; and added a new module-level function eta_poly_relations which finds polynomial relations between two eta products.
Excellent -- I will review this right away. John
comment:7 Changed 14 years ago by
Summary: | [with patch, with partial review] Eta product modular functions → [with new patch, with positive review apaert from some minor issues] Eta product modular functions |
---|
Review summary: This is a great piece of work, especially after the second patch. I have made a few changes, detailed below, which are in the 3rd patch.
- I changed the type-checking of the level parameter so that it forces it to be a Sage integer. This is a good idea since some integer methods would not work if the level was a Python integer, and it also allows you to say
EtaGroup_class(4/2)
should you ever want to.
- docstring for !EtaGroup_class.basis() had a spurious redundant INPUT line (N)
- I added a little more type and value checking of parameters
- In eta_poly_relations there is a lot of output which cannot be turned off. Why not include a parameter verbose, default False, and only print the output if it is True? I put this in and changed the doctests accordingly.
- I commented out the NotImplemented plot function, as it seemed pointless to have it!
Coverage test gives this:
./sage -coverage devel/sage-eta/sage/modular/etaproducts.py devel/sage-eta/sage/modular/etaproducts.py ERROR: Please define a s == loads(dumps(s)) doctest. SCORE devel/sage-eta/sage/modular/etaproducts.py: 74% (23 of 31) Missing documentation: * __init__(self, level) * _repr_(self) * __call__(self, dict) * __init__(self, parent, rdict) * __cmp__(self, other) * __eq__(self, other) * _short_repr(self) * _eta_relations_helper(eta1, eta2, degree, qexp_terms, labels, verbose) Possibly wrong (function name doesn't occur in doctests): * _mul_(self, other) * _div_(self, other) * _repr_(self) * _repr_(self)
I have never been sure about the loads(dumps) message. Apart from that all functions which are not preceded by an underscore have docstrings and doctests, which is the main thing: it would be better if they had a little documentation, but then you'll get the complaint that they should also have doctests.
Conclusion: I am very happy with this, but it would be even better if someone who knows a lot more about eta products (such as Ken McMurdy) could put it through its paces.
comment:8 Changed 14 years ago by
Summary: | [with new patch, with positive review apaert from some minor issues] Eta product modular functions → [with new patch, needs work] Eta product modular functions |
---|
Ok, anything going on here? The patches here have been bitrotting for a while and it would be nice if someone wrote the missing doctests. In order to have the right report pick it up I am renaming it to "needs work".
Cheers,
Michael
comment:9 Changed 14 years ago by
David,
If you are happy with the additions I made then we can get this accepted soon. I have not yet tried applying the patches to 3.1.3.*. John
comment:10 Changed 14 years ago by
I'm very happy with John's changes. I've sent a copy to Ken McMurdy? for review; he said he'll reply by September 26th. I doubt that bit rot will be a problem with this one as it's a single new source file which is entirely self-contained.
I'll get to work on the missing doctests and post a new patch.
David
Changed 14 years ago by
Attachment: | 10295.patch added |
---|
comment:11 Changed 14 years ago by
Summary: | [with new patch, needs work] Eta product modular functions → [with patches; under review by Ken McMurdy] Eta product modular functions |
---|
I've added doctests for the underscore methods, and a loads(dumps()) test; sage -coverage now returns 100%.
comment:12 Changed 14 years ago by
Excellent! I just checked that the sequence of 4 patches applies cleanly to 3.1.3.alpha0, and all tests in sage/modular pass.
Let it roll: even if Ken McMurdy? suggests changes and additions, why not merge this now?
I'll inform Lloyd, Gabor and Lassina about this once it is merged, since it sefinitely a success story coming out of their August workshop.
comment:13 Changed 14 years ago by
Summary: | [with patches; under review by Ken McMurdy] Eta product modular functions → [with patches; positive review] Eta product modular functions |
---|
Cremona gave this a positive review (above), so I'm changing the heading.
comment:14 Changed 14 years ago by
Milestone: | sage-feature → sage-3.2 |
---|
I'm also changing this to target 3.2
comment:15 Changed 14 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
Merged all four patches in Sage 3.2.alpha1
comment:16 Changed 13 years ago by
Authors: | → David Loeffler |
---|---|
Merged in: | → 3.2.alpha1 |
Reviewers: | → John Cremona |
First version: patch against sage 3.1.1