Opened 14 years ago

Closed 14 years ago

Last modified 13 years ago

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

Status badges

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)

10292.patch (18.7 KB) - added by David Loeffler 14 years ago.
First version: patch against sage 3.1.1
10293.patch (26.3 KB) - added by David Loeffler 14 years ago.
10294.patch (8.3 KB) - added by John Cremona 14 years ago.
Apply after the previous two patches
10295.patch (6.4 KB) - added by David Loeffler 14 years ago.

Download all attachments as: .zip

Change History (20)

comment:1 Changed 14 years ago by David Loeffler

Milestone: sage-feature
Status: newassigned

Changed 14 years ago by David Loeffler

Attachment: 10292.patch added

First version: patch against sage 3.1.1

comment:2 Changed 14 years ago by David Loeffler

Summary: Eta product modular functions[with patch, needs review] Eta product modular functions

comment:3 Changed 14 years ago by John Cremona

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 David Loeffler

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 David Loeffler

Attachment: 10293.patch added

comment:5 Changed 14 years ago by David Loeffler

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 in reply to:  5 Changed 14 years ago by John Cremona

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

Changed 14 years ago by John Cremona

Attachment: 10294.patch added

Apply after the previous two patches

comment:7 Changed 14 years ago by John Cremona

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 Michael Abshoff

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 John Cremona

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 David Loeffler

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 David Loeffler

Attachment: 10295.patch added

comment:11 Changed 14 years ago by David Loeffler

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 John Cremona

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 William Stein

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 William Stein

Milestone: sage-featuresage-3.2

I'm also changing this to target 3.2

comment:15 Changed 14 years ago by Michael Abshoff

Resolution: fixed
Status: assignedclosed

Merged all four patches in Sage 3.2.alpha1

comment:16 Changed 13 years ago by David Loeffler

Authors: David Loeffler
Merged in: 3.2.alpha1
Reviewers: John Cremona
Note: See TracTickets for help on using tickets.