#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)
Change History (26)
comment:1 Changed 10 years ago by
comment:2 Changed 10 years ago by
Thanks, I'll review it as soon as possible (I wrote the function so it is my fault!)
comment:3 Changed 10 years ago by
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
- Status changed from new to needs_review
comment:5 Changed 10 years ago by
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
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 E^{0} (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
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
- 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
- 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
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: ↓ 13 Changed 10 years ago by
- Cc robertwb added
comment:12 follow-up: ↓ 14 Changed 10 years ago by
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
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: ↓ 16 Changed 10 years ago by
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
Patch trac_7935b.patch adds the #random tag.
comment:16 in reply to: ↑ 14 Changed 10 years ago by
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.
comment:17 Changed 10 years ago by
- 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
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
I will rebase it. Something else must have merged since which messed up line numbers or something.
comment:20 Changed 10 years ago by
I have rebased it.
comment:21 Changed 10 years ago by
- 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
- 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
I think we can leave it as it is although 99% of the work is by Chris.
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.