Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#7935 closed defect (fixed)

local_data for elliptic curves over number fields

Reported by: wuthrich Owned by: cremona
Priority: major Milestone: sage-4.3.3
Component: elliptic curves Keywords: elliptic curve, number fields, local data, tamagawa
Cc: cremona, robertwb Merged in: sage-4.3.3.alpha0
Authors: Chris Wuthrich, John Cremona Reviewers: John Cremona, Chris Wuthrich, Robert Miller
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

First of all, I spotted the following bug:

K.<a> = NumberField(x^2-38)
E = EllipticCurve([a,1/2])
E.global_integral_model()

which yields

AssertionError: bug in global_integral_model: [0, 0, 0, 4/361*a, 4/6859]

and that is easy to fix. But I also wish to add a tamagawa_product and improve the documentation.

I will post a patch later today.

Attachments (3)

trac_7935.patch (13.9 KB) - added by wuthrich 10 years ago.
exported against 4.3.alpha1
trac_7935b.patch (2.4 KB) - added by cremona 10 years ago.
Apply after previous
trac_7935b.2.patch (2.8 KB) - added by wuthrich 10 years ago.
second patch to be applied after the first patch, exported against 4.3.1

Download all attachments as: .zip

Change History (26)

comment:1 Changed 10 years ago by wuthrich

Before I get complains, I'd better add that tamagawa_product will not be the product of tamagawa numbers, but rather it envolves also the changes to the local minimal model. It will be the term that appears in BSD. Something I always wanted to add.

comment:2 Changed 10 years ago by cremona

Thanks, I'll review it as soon as possible (I wrote the function so it is my fault!)

Changed 10 years ago by wuthrich

exported against 4.3.alpha1

comment:3 Changed 10 years ago by wuthrich

I am not sure whether it is a good idea to have a definition of tamagawa_product for number fields that differs from the one for Q; but it is the only sensible definition. An option whould be to change the definition over Q; it would affect only non-minimal equation, but still it might break a lot of compatibility.

The very last thing in the patch, line 1024 in the new ell_number_field.py, is strange. It yields now a different global minimal model (yes they differ by a weierstrass-change with u a unit in K). If one computes this outside of this file one gets the old result. This may be linked to #7930, I don't know.

comment:4 Changed 10 years ago by wuthrich

  • Status changed from new to needs_review

comment:5 Changed 10 years ago by cremona

Thanks for all the excellent documentation!

About the bug: I am puzzled by this, as I still don't see how either my original code failed or how the change (of uniformiser) works best. Here was my thinking: if val(a_i)<0 for some i, then find the minimal e such that e*i+valuation(a_i) is >=0 for i in [1,2,3,4,6] and then replace a_i by a_i/pi^(e*i). The point about using the "negative" flag on the uniformiser was so ensure that dividing by a power of pi maintained integrality at all other primes. Your new code will not do that.

Would this be better:

        D = self.discriminant()
        for P in D.prime_factors():
            if not all([a.valuation(P)>=0 for a in ai]):
                   pi=K.uniformizer(P,'negative')
                   e  = min([(ai[i].valuation(P)/[1,2,3,4,6][i]) for i in range(5)]).floor()
                   ai = [ai[i]/pi**(e*[1,2,3,4,6][i]) for i in range(5)]

?

I have not looked at your last point yet. I am not so keen on tamagawa_product() not giving the product of the Tamagawa numbers. Are you sure that over Q when an equation is non-minimal at p that the number returned by tamagawa_number() is not the index for the minimal model? I thought that was what Tate's algorithm would give. I'll need to look at it more closely.

I am leaving this as "needs review" as I have not actually tested it yet!

comment:6 Changed 10 years ago by wuthrich

Maybe you make a little sign error ? In the bug-example, the valuation of a6 at P = (a) is -2. So e is -1. So you are really multiplying the ais by pi+i. So pi should be integral away from P.

As to your comment on tamagawa_product(), we have

sage: E = EllipticCurve('11a2')
sage: E.tamagawa_product()
1

sage: E2 = E.change_weierstrass_model([1/11,0,0,1])
sage: E2
Elliptic Curve defined by y^2 + 3993*y = x^3 - 121*x^2 - 114492620*x - 466951591502 over Rational Field
sage: E2.tamagawa_product()
1

So it is defined to be the product of the Tamagawa numbers (i.e. the index of E0 (K_v)) on the minimal model.

My definition (and that is the only sensible over number fields) of tamagawa_product() changes as one changes the chosen invariant differential in such a way that the product of it with the periods is invariant under this choice. In particular it is not the product of the Tamagawa numbers despite the name.

comment:7 Changed 10 years ago by cremona

OK, I agree that there was a sign error... The value of e in the code is always negative (since we skip if a is integral, hence at least one of the fractions (which are rounded down by the "/" operator) is negative. Hence we divide by a negative power of pi, i.e. multiply by a positive power, so haveing pi integral is exactly what you want. You win!

Sorry, it's 5.30 on a Friday after the first week of term.

On the T. products, can we come up with a different name for one of the functions since they don't agree? Either that, of have an optional parameter in the case over Q to give the same value as if Q were treated as a number field?

comment:8 Changed 10 years ago by cremona

  • Status changed from needs_review to positive_review

Positive review: applies fine to 4.3.1.alpha2, all tests in sage/schemes/elliptic_curves pass, docs build well (there's a lot of useful extra documentation in the patch), and I agree with the design decisions made.

Thanks for fixing the bug in my code (and more!)

comment:9 Changed 10 years ago by rlm

  • Status changed from positive_review to needs_work
File "sage/schemes/elliptic_curves/ell_number_field.py", line 1026:
    sage: E2
Expected:
    Elliptic Curve defined by y^2 + a*x*y + (a+1)*y = x^3 + (a+1)*x^2 + (12289755603565800754*a-75759141535687466985)*x + (51556320144761417221790307379*a-317814501841918807353201512829) over Number Field in a with defining polynomial x^2 - 38
Got:
    Elliptic Curve defined by y^2 + a*x*y + (a+1)*y = x^3 + (a+1)*x^2 + (368258520200522046806318444*a-2270097978636731786720859345)*x + (8456608930173478039472018047583706316424*a-52130038506793883217874390501829588391299) over Number Field in a with defining polynomial x^2 - 38
**********************************************************************

comment:10 Changed 10 years ago by cremona

This is very strange. For that curve E2, I sometimes get

Elliptic Curve defined by y^2 + a*x*y + (a+1)*y = x^3 + (a+1)*x^2 + (12289755603565800754*a-75759141535687466985)*x + (51556320144761417221790307379*a-317814501841918807353201512829) over Number Field in a with defining polynomial x^2 - 38

but sometimes I get

Elliptic Curve defined by y^2 + a*x*y + (a+1)*y = x^3 + (a+1)*x^2 + (368258520200522046806318444*a-2270097978636731786720859345)*x + (8456608930173478039472018047583706316424*a-52130038506793883217874390501829588391299) over Number Field in a with defining polynomial x^2 - 38

on the same machine. (Note that both are valid, since both are models with a unit Discriminant so are certainly global minimal models.)

The only way round this I can see in the short term is to remove the line in the doctest which displays E2 itself. What do people think?

comment:11 follow-up: Changed 10 years ago by wuthrich

  • Cc robertwb added

I was afraid that this would happen. I strongly believe that this is the same bug as #7930. Both answers are valid global minimal models (as mentionned earlier), I could propose that we put a #random behind it and wait for #7930 to be solved.

comment:12 follow-up: Changed 10 years ago by wuthrich

As we are back to review: What about another name for tamagawa_product. It seems not a bad name for what it is, but I admit that John objection is a fair one. What other names could we give it ?

comment:13 in reply to: ↑ 11 Changed 10 years ago by cremona

Replying to wuthrich:

I was afraid that this would happen. I strongly believe that this is the same bug as #7930. Both answers are valid global minimal models (as mentionned earlier), I could propose that we put a #random behind it and wait for #7930 to be solved.

I'll go with that. I'll even put in a comment after the #random saying (minimal model is not unique).

comment:14 in reply to: ↑ 12 ; follow-up: Changed 10 years ago by cremona

Replying to wuthrich:

As we are back to review: What about another name for tamagawa_product. It seems not a bad name for what it is, but I admit that John objection is a fair one. What other names could we give it ?

Howe about bsd_tamagawa_product() or minimal_tamagawa_product()?

comment:15 Changed 10 years ago by cremona

Patch trac_7935b.patch adds the #random tag.

comment:16 in reply to: ↑ 14 Changed 10 years ago by wuthrich

How about bsd_tamagawa_product() or minimal_tamagawa_product()?

the first one would be ok, the second is missleading as it is not minimal.

tamagawa_product_bsd or tamagawa_product_as_in_bsd so that the tab still completes it as before.

Changed 10 years ago by cremona

Apply after previous

comment:17 Changed 10 years ago by cremona

  • Status changed from needs_work to needs_review

The new version of trac_7935b.patch changes the name to tamagawa_product_bsd .

Chris, if you are happy with this second patch now you can give it a positive review.

comment:18 Changed 10 years ago by wuthrich

Sorry, John, I could not apply your patch, the last hunk failed. Maybe because I did not upgrade yet. I try again later.

comment:19 Changed 10 years ago by cremona

I will rebase it. Something else must have merged since which messed up line numbers or something.

Changed 10 years ago by wuthrich

second patch to be applied after the first patch, exported against 4.3.1

comment:20 Changed 10 years ago by wuthrich

I have rebased it.

comment:21 Changed 10 years ago by cremona

  • Status changed from needs_review to positive_review

Sorry for the delay. Apply the first and third patches to 4.3.1 -- all works.

comment:22 Changed 10 years ago by mpatel

  • Authors set to Chris Wuthrich, John Cremona
  • Merged in set to sage-4.3.3.alpha0
  • Resolution set to fixed
  • Reviewers set to John Cremona, Chris Wuthrich, Robert Miller
  • Status changed from positive_review to closed

Please update the Author and Reviewer fields, if they're wrong.

comment:23 Changed 10 years ago by cremona

I think we can leave it as it is although 99% of the work is by Chris.

Note: See TracTickets for help on using tickets.