Opened 4 years ago

Closed 4 years ago

#23599 closed enhancement (fixed)

making local_data and check_prime from Elliptic curves to accept P in QQ

Reported by: edgarcosta Owned by:
Priority: minor Milestone: sage-8.1
Component: elliptic curves Keywords: days88
Cc: cremona Merged in:
Authors: Edgar Costa Reviewers: Aly Deines
Report Upstream: N/A Work issues:
Branch: 58b7fbd (Commits, GitHub, GitLab) Commit: 58b7fbd209d8d7f3850df4ab6f20eeaf3e7f5e2a
Dependencies: Stopgaps:

Status badges

Description

I noticed that one could do.

E = EllipticCurve(QQ, "35a2")
E.local_data(ZZ(5)); 

but not

E.local_data(QQ(5));

However, if we base change to a number field K, it works if we pass an element of K

K.<i> = NumberField(x^2+1)
E = EllipticCurve([1 + i, 0, 1, 0, 0])
E.local_data(2*i + 1)
E.local_data(K(3))

but

E.local_data(3)

woudln't.

I tried to fix that.

ps: the right implementation here should be along the lines,

P = K.fractional_ideal(P)
if P.is_prime():
    if K is QQ:
        return P.gen();
    else:
        return P;

but QQ doesn't have fractional ideals, see #23595

Change History (10)

comment:1 Changed 4 years ago by edgarcosta

  • Status changed from new to needs_review

comment:2 Changed 4 years ago by wuthrich

Not that I can claim to understand types in sage completely, but your change:

-        if isinstance(P, integer_types + (Integer,)):
+        if P in ZZ or isinstance(P, integer_types + (Integer,)):

seems to complicate a line without changing the criterion, no?

comment:3 Changed 4 years ago by wuthrich

Ah, now I see. Of course that is what you want, P=QQ(3) gives now True. Sorry for the noise.

comment:4 Changed 4 years ago by cremona

I approve of these changes (in principle, I have not looked at the details). There are still many places where QQ (and elements of QQ) are not treated the same as for number fields, and missing methods for QQ and rationals which exist for general number fields. I don't think it will ever be possible to eliminate these inconsistencies globally for a number of reasons:

  1. We have different types (classes) for elements of QQ and ZZ, and that will surely stay true, while we have no separate type for an integral element of a number field.
  2. Number theorists are quite happy with K.ideal(2) returning a fractional ideal -- which is non-trivial! -- but QQ.ideal(2) returns the unit ideal. Basically that's because algebraists are pedantic and say quite correctly that the ideal of QQ generated by 2 is the unit ideal since 2 is a unit in the field. Of course we can do ZZ.ideal(2) to get the ideal; but not ZZ.ideal(1/2), i.e. we have integral ideals of ZZ but not fractional ones. So this is in fact related to the distinct classes for ZZ and QQ but none for rings of integers generally.

Hence (in conclusion!) the best we can do is tinker with code in the way this ticket does so that natural constructions work as widely as possible. Thanks, Edgar.

comment:5 Changed 4 years ago by jen

  • Keywords days88 added

comment:6 Changed 4 years ago by aly.deines

  • Branch changed from u/edgarcosta/local_data to u/aly.deines/local_data

comment:7 Changed 4 years ago by aly.deines

  • Commit changed from 5976388e6527d821158d37796c5251608f64de00 to 58b7fbd209d8d7f3850df4ab6f20eeaf3e7f5e2a
  • Reviewers set to Aly Deines
  • Status changed from needs_review to positive_review

Merged.

Plays well. All tests pass. Documentation looks good.


New commits:

58b7fbdMerge branch 'u/edgarcosta/local_data' of git://trac.sagemath.org/sage into t/23599/local_data

comment:8 Changed 4 years ago by vbraun

  • Status changed from positive_review to needs_work

Author name missing...

comment:9 Changed 4 years ago by edgarcosta

  • Authors set to Edgar Costa
  • Status changed from needs_work to positive_review

comment:10 Changed 4 years ago by vbraun

  • Branch changed from u/aly.deines/local_data to 58b7fbd209d8d7f3850df4ab6f20eeaf3e7f5e2a
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.