Opened 4 months ago

Closed 4 months ago

#31433 closed enhancement (fixed)

more efficient method for number of real components of an elliptic curve over Q

Reported by: cremona Owned by:
Priority: major Milestone: sage-9.3
Component: elliptic curves Keywords: elliptic curve
Cc: slelievre Merged in:
Authors: John Cremona Reviewers: Frédéric Chapoton
Report Upstream: N/A Work issues:
Branch: 41b446c (Commits, GitHub, GitLab) Commit: 41b446c04c4a895e2797f7f21462cff1ee8bd369
Dependencies: Stopgaps:

Status badges

Description (last modified by slelievre)

Define an elliptic curve, find its discriminant and its number of real components:

sage: E = EllipticCurve([0, -1, 1, 0, 0])
sage: E.discriminant()
-11
sage: E.real_components()
1

The number of real components is 1 or 2 when the discriminant is negative or positive respectively. The current code does a lot of unnecessary work:

        invs = self.short_weierstrass_model().ainvs()
        x = rings.polygen(self.base_ring())
        f = x**3 + invs[3]*x + invs[4]
        if f.discriminant() > 0:
            return 2
        else:
            return 1

It is unnecessary to compute a short Weierstrass model, take its coefficients, construct a polynomial, and compute its discriminant, since E.discriminant() has the same sign as the discriminant computed here.

As well as fixing this we add a real_components() method for elliptic curves over number fields, which takes as a parameter a real embedding of the base field.

Change History (8)

comment:1 Changed 4 months ago by slelievre

  • Cc slelievre added
  • Component changed from PLEASE CHANGE to elliptic curves

comment:2 Changed 4 months ago by cremona

  • Branch set to u/cremona/31433
  • Commit set to 2f0b0c1e73878dde0133d9786ee22eda89816708
  • Status changed from new to needs_review

New commits:

f3c6f63#31433: simplify real_components method for elliptic curves over QQ
2f0b0c1#31433: add real_components method for elliptic curves over number fields

comment:3 Changed 4 months ago by slelievre

  • Description modified (diff)

comment:4 Changed 4 months ago by chapoton

two many final dots here:

+        Return the number of real components with respect to a real embedding of the base field..

comment:5 Changed 4 months ago by git

  • Commit changed from 2f0b0c1e73878dde0133d9786ee22eda89816708 to 41b446c04c4a895e2797f7f21462cff1ee8bd369

Branch pushed to git repo; I updated commit sha1. New commits:

41b446c#31433: delete extra dot

comment:6 follow-up: Changed 4 months ago by chapoton

  • Reviewers set to Frédéric Chapoton
  • Status changed from needs_review to positive_review

ok, looks good. Avanti !

comment:7 in reply to: ↑ 6 Changed 4 months ago by cremona

Replying to chapoton:

ok, looks good. Avanti !

Merci!

comment:8 Changed 4 months ago by vbraun

  • Branch changed from u/cremona/31433 to 41b446c04c4a895e2797f7f21462cff1ee8bd369
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.