Opened 10 years ago

Closed 7 years ago

Last modified 7 years ago

#11630 closed defect (fixed)

Local data of elliptic curves should not do any global work

Reported by: wuthrich Owned by:
Priority: major Milestone: sage-6.1
Component: elliptic curves Keywords: Tate's algorithm
Cc: cremona Merged in:
Authors: Chris Wuthrich Reviewers: John Cremona
Report Upstream: N/A Work issues:
Branch: u/wuthrich/ticket/11630 (Commits, GitHub, GitLab) Commit: d2d345764892fceb13f037d63adb9bd837ea6fde
Dependencies: Stopgaps:

Status badges

Description

... unless asked to do so.

I am talking about the line 687 in sage.scheme.elliptic_curves_ell_local_data which reads

        principal_flag = P.is_principal()
        if principal_flag:
            pi = P.gens_reduced()[0]
            verbose("P is principal, generator pi = %s"%pi, t, 1)
        else:
            pi = K.uniformizer(P, 'positive')
            verbose("P is not principal, uniformizer pi = %s"%pi, t, 1)

While it can be useful, especially when one wants a global minimal model, it is often very harmful. If the class group of the field is huge or difficult to compute, one will not be able to determine the fibres of the Neron model, simply because of this line.

One should add a switch which is by default set to use the second case and only if needed to the first case.

Of course, in an ideal world Tate's algorithm should be implemented for any elliptic curve over a local field, rather than a number field.

Attachments (1)

trac_11630_localize_tates_algorithm.patch (8.2 KB) - added by wuthrich 8 years ago.
exported against 5.9

Download all attachments as: .zip

Change History (17)

comment:1 Changed 10 years ago by cremona

That is a sensible suggestion, and if you write a patch I'll review it! Ideally I would like the code to say "if you know that P is principal and have a generator, the use it, otherwise...", but I suppose that is what the uniformizer function might do anyway.

I think that even if we did have a version for Tate's algorithm which applied directly to an elliptic curve over a local field, I would still want a global version -- which used generators of prime ideals for fields of class number 1 at least!

comment:2 Changed 10 years ago by wuthrich

  • Authors set to Chris Wuthrich
  • Status changed from new to needs_review

This patch does what I suggested + it also changes global_minimal_model. This function was only implemented when the class number was 1, now it tries to change to a global minimal model in any field.

The patch should be applied after #11540.

comment:3 Changed 10 years ago by cremona

  • Reviewers set to John Cremona
  • Status changed from needs_review to needs_work
  • Work issues set to rebase to 4.7.1

First a technical point: the patch does not apply to 4.7.1.rc1+#11540. Since 4.7.1 is almost out could you rebase the patch to the latest release candidate?

I'll test more after that. Meanwhile I am not sure that the logic is correct in the new, more general, global_minimal_model() function. Are you assuming that the is a global minimal modell iff after replacing with a local minimal model at every relevant prime, one after the other, the result is minimal at all primes? I'm sure that is not right. On my (infinitely long) to-do list is a function that determines whether there is a global minimal model for any curve, which comes down to computing a certain ideal and testing if it is principal; if so then (using a generator for that ideal) one can find the minimal model. That should be done, but not on this ticket.

comment:4 Changed 10 years ago by wuthrich

  • Owner changed from cremona to (none)

Thanks, John, for the quick review and for spotting my stupid error. In fact all I really wanted was that global minimal models are recognised when they are, but that was a bad idea. I agree with your comments and with your suggestion to open another ticket for this. This is ticket #11697 .

I will rebase it today, hopefully, and delete the relevant passages. (Sorry, but I have decided quite a while ago not to work with intermediate versions anymore, but now 4.7.1 is here.)

comment:5 Changed 10 years ago by wuthrich

  • Status changed from needs_work to needs_review

Ok. I rebased it and I removed the changes in global_minimal_model. The patch should be applied after #11540.

comment:6 Changed 9 years ago by wuthrich

The patch still applies to 4.7.2.

comment:7 Changed 9 years ago by davidloeffler

  • Reviewers changed from John Cremona to John Cremona, PatchBot
  • Status changed from needs_review to needs_work
  • Work issues changed from rebase to 4.7.1 to rebase to 5.0

The patch does not apply to 4.8 or to 5.0.beta7, according to the patchbot.

comment:8 Changed 8 years ago by wuthrich

  • Status changed from needs_work to needs_review

I rebased the patch. I exported it after appying #12509 and #13953, but maybe that is no problem.

The new patch includes a lot of deletion of trailing white space in the two edited files. Not sure that this is desired.

Changed 8 years ago by wuthrich

exported against 5.9

comment:9 Changed 8 years ago by wuthrich

I have removed the whitespaces. This patch should apply to 5.9 + #12509.

comment:10 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:11 Changed 7 years ago by wuthrich

  • Branch set to u/wuthrich/ticket/11630
  • Modified changed from 08/13/13 15:35:53 to 08/13/13 15:35:53

comment:12 Changed 7 years ago by wuthrich

  • Commit set to d2d345764892fceb13f037d63adb9bd837ea6fde

That is my first use of the new git-stuff. I hope I did the right thing.

I simply rebased the patch. It is ready for a review.


New commits:

d2d3457trac: #11630. Further changes.
1775e28trac 11630: tate's algorithm should avoid global computations unless asked to do so

comment:13 Changed 7 years ago by cremona

I have put this onto my to-do list to attempt my first git-style review, but in view of the season do not promise to do it in the next few days. [8 days later] Under review!

Last edited 7 years ago by cremona (previous) (diff)

comment:14 Changed 7 years ago by cremona

  • Status changed from needs_review to positive_review

I am giving this a positive review despite the patchbot's red blob.

comment:15 Changed 7 years ago by vbraun

  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:16 Changed 7 years ago by jdemeyer

  • Reviewers changed from John Cremona, PatchBot to John Cremona
  • Work issues rebase to 5.0 deleted
Note: See TracTickets for help on using tickets.