Opened 14 years ago

Closed 14 years ago

Last modified 14 years ago

#3934 closed enhancement (fixed)

[with patches; positive review] Eta product modular functions

Reported by: davidloeffler Owned by: davidloeffler
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:

GitHub link to the corresponding issue

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 davidloeffler 14 years ago.
First version: patch against sage 3.1.1
10293.patch (26.3 KB) - added by davidloeffler 14 years ago.
10294.patch (8.3 KB) - added by cremona 14 years ago.
Apply after the previous two patches
10295.patch (6.4 KB) - added by davidloeffler 14 years ago.

Download all attachments as: .zip

Change History (20)

comment:1 Changed 14 years ago by davidloeffler

Milestone: sage-feature
Status: newassigned

Changed 14 years ago by davidloeffler

Attachment: 10292.patch added

First version: patch against sage 3.1.1

comment:2 Changed 14 years ago by davidloeffler

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

comment:3 Changed 14 years ago by 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 davidloeffler

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 davidloeffler

Attachment: 10293.patch added

comment:5 Changed 14 years ago by davidloeffler

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 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 cremona

Attachment: 10294.patch added

Apply after the previous two patches

comment:7 Changed 14 years ago by 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 mabshoff

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 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 davidloeffler

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 davidloeffler

Attachment: 10295.patch added

comment:11 Changed 14 years ago by davidloeffler

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 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 was

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 was

Milestone: sage-featuresage-3.2

I'm also changing this to target 3.2

comment:15 Changed 14 years ago by mabshoff

Resolution: fixed
Status: assignedclosed

Merged all four patches in Sage 3.2.alpha1

comment:16 Changed 14 years ago by davidloeffler

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