Opened 12 years ago

Closed 11 years ago

#6863 closed enhancement (fixed)

Implement cusps over number fields

Reported by: cremona Owned by: craigcitro
Priority: major Milestone: sage-4.3.2
Component: modular forms Keywords: number field modular
Cc: mtaranes Merged in: sage-4.3.2.alpha0
Authors: Maite Aranes, John Cremona Reviewers: Craig Citro, John Cremona
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

This is related to, but not dependent on, #6829.

Cusps over a number field K are elements of P^1(K). Support for these is to be provided as part of the general implementation of modular symbols and related matters over number fields.

A patch which will be available shortly will supply this implementation, together with related utilities such as testing Gamma_0(N)-equivalence etc.

This work has been done by Maite Aranes, under the supervision of John Cremona.

Attachments (1)

cusps_nf.patch (42.9 KB) - added by mtaranes 12 years ago.
Patch based on 4.1.1

Download all attachments as: .zip

Change History (9)

Changed 12 years ago by mtaranes

Patch based on 4.1.1

comment:1 Changed 12 years ago by mtaranes

  • Summary changed from Implement cusps over number fields to [with patch, needs review] Implement cusps over number fields

comment:2 Changed 11 years ago by cremona

  • Report Upstream set to N/A

I just checked that the patch applies fine to 4.3.alpha 1, and it does.

comment:3 Changed 11 years ago by craigcitro

  • Status changed from needs_review to needs_work
  • Summary changed from [with patch, needs review] Implement cusps over number fields to Implement cusps over number fields

So this code looks really nice -- I'm really happy to see this functionality in Sage, and I'm very impressed with the completeness of the functionality, and the documentation/doctests.

However, I think this code does need some purely cosmetic changes before we merge it. Here are my first thoughts:

  • I think that we shouldn't provide a separate "NFCusp" function to the user -- they should just call Cusp, which could deduce the base ring from the arguments, and take an optional base_ring= parameter.
  • For that matter, we should probably just have a Cusp_rationals and Cusp_number_field class which both inherit from a Cusp_generic, and unify as much of this code as possible with the existing cusps code.
  • Similar comments to the previous two also apply to NFCusps, i.e. the parent for cusps.
  • I really like that there are convenient functions for getting a corresponding number field element from a cusp -- however, we should probably do the extra work to make the coercion framework know about these, so that you could simply do something like K(c) to get a representative for the cusp.
  • In fact, I might go one step further and say that we shouldn't really need to add any new functions into the global namespace -- i.e. sage/modular/all.py shouldn't need to import anything directly, except maybe the _clear_cache functions. All of the things that do get imported are analogues for number fields of existing bits, which should be able to be handled transparently. As far as the _clear_cache stuff, maybe we could just have those be available as methods on the parents? I wouldn't mind _Cusp_clear_cache_ or something at top-level, as long as it was prefixed with _.

I think that's it at a first go, but it's probably enough to get started moving in the right direction. John and/or Maite, do you want to make these changes, or would you rather I do it? I won't get to it for about two weeks, but I'm happy to do it if you won't have time before then.

comment:4 Changed 11 years ago by cremona

Craig, Thanks for your comments. Since it's quite a long time since Maite actually wrote this I don't think we are in a hurry to start reorganising the code as you suggest. I think that, while we realised that the best thing in the long term would be to unify code as you suggest, we did not want to do anything which made classical cusps slower, hence the parallel implementation. So if you unify the cusps code (which I would like to see in the long run) it will be necessary to test carefully that there is no speed regression for Q-cusps.

comment:5 Changed 11 years ago by cremona

  • Status changed from needs_work to needs_review

On second thoughts, why don't we let this code get merged now, and make a new ticket for the refactoring? That way this code will not languish any longer just in case we don't get around to making your suggested improvements for a while.

To assist this process I'm putting it back to "needs review". Craig, you can signal your answer by either moving it back to "needs work" or on to "positive review" :)

comment:6 follow-up: Changed 11 years ago by craigcitro

  • Authors set to Maite Aranes, John Cremona
  • Reviewers set to Craig Citro, John Cremona
  • Status changed from needs_review to positive_review

Yeah, that's a good point. I'd rather have this code merged and waiting on a cleanup than have it sitting and waiting for the same cleanup. I've made #8015 as a reminder to merge these two.

(John, I'm listing you as a reviewer because I know you reviewed much of the content of this code as it was being developed.)

comment:7 in reply to: ↑ 6 Changed 11 years ago by cremona

Replying to craigcitro:

Yeah, that's a good point. I'd rather have this code merged and waiting on a cleanup than have it sitting and waiting for the same cleanup. I've made #8015 as a reminder to merge these two.

(John, I'm listing you as a reviewer because I know you reviewed much of the content of this code as it was being developed.)

That's fair. My involvement should be recorded somewhere, but it was Maite who did all the code writing.

Thanks for the review!

comment:8 Changed 11 years ago by mvngu

  • Merged in set to sage-4.3.2.alpha0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.