Ticket #6402 (closed defect: fixed)

Opened 4 years ago

Last modified 4 years ago

[with patch, positive review] Fix bugs + improve documentation for overconvergent modular forms

Reported by: davidloeffler Owned by: craigcitro
Priority: major Milestone: sage-4.1.2
Component: modular forms Keywords:
Cc: was Work issues:
Report Upstream: Reviewers: Alex Ghitza, Minh Van Nguyen
Authors: David Loeffler Merged in: Sage 4.1.2.alpha0
Dependencies: Stopgaps:

Description

This patch fixes lots of bugs + adds a substantial amount of documentation and examples, based on my talk at Sage Days 16.

Attachments

trac_6402-overconvergent_fixes.patch Download (18.1 KB) - added by davidloeffler 4 years ago.
patch against 4.0.2
trac_6402-referee.patch Download (10.3 KB) - added by AlexGhitza 4 years ago.
apply after the previous patch
trac_6402-typos.patch Download (1.5 KB) - added by mvngu 4 years ago.
reviewer patch: more typo fixes

Change History

Changed 4 years ago by davidloeffler

patch against 4.0.2

comment:1 Changed 4 years ago by davidloeffler

Here's a patch.

comment:2 Changed 4 years ago by davidloeffler

  • Summary changed from Fix bugs + improve documentation for overconvergent modular forms to [with patch, needs review] Fix bugs + improve documentation for overconvergent modular forms

comment:3 Changed 4 years ago by davidloeffler

  • Milestone changed from sage-feature to sage-4.1.1

comment:4 Changed 4 years ago by AlexGhitza

Looks good, and the added documentation is really great.

I'm attaching a tiny referee patch fixing a few typos, and if you are ok with it we can give this a positive review.

There is one small issue, which I leave up to you whether you want to fix it or not: in the method valuation_plot, you added an optional argument rmax, but the docstring says nothing about its role.

Changed 4 years ago by AlexGhitza

apply after the previous patch

comment:5 Changed 4 years ago by davidloeffler

  • Reviewers set to Alex Ghitza
  • Summary changed from [with patch, needs review] Fix bugs + improve documentation for overconvergent modular forms to [with patch, positive review] Fix bugs + improve documentation for overconvergent modular forms
  • Authors set to David Loeffler

Fine by me.

Changed 4 years ago by mvngu

reviewer patch: more typo fixes

comment:6 Changed 4 years ago by mvngu

  • Summary changed from [with patch, positive review] Fix bugs + improve documentation for overconvergent modular forms to [with patch, needs review] Fix bugs + improve documentation for overconvergent modular forms

The reviewer patch trac_6402-typos.patch fixes some typos left over from the previous two patches. If that is OK, then the whole ticket can be merged.

comment:7 Changed 4 years ago by davidloeffler

  • Summary changed from [with patch, needs review] Fix bugs + improve documentation for overconvergent modular forms to [with patch, positive review] Fix bugs + improve documentation for overconvergent modular forms

I certainly have no problems with Minh's suggestions. Can I suggest we get this in soon, rather than fixing every conceivable micro-bug, since the initial patch fixes some rather major and embarassing bugs that render the whole module basically unusable?

comment:8 Changed 4 years ago by mvngu

  • Status changed from new to closed
  • Reviewers changed from Alex Ghitza to Alex Ghitza, Minh Van Nguyen
  • Resolution set to fixed
  • Merged in set to Sage 4.1.2.alpha0

comment:9 Changed 4 years ago by mvngu

Merged all three patches in this order:

  1. trac_6402-overconvergent_fixes.patch
  2. trac_6402-referee.patch
  3. trac_6402-typos.patch
Note: See TracTickets for help on using tickets.