Add CM detection for elliptic curves over number fields

[JohnCremona]

Elliptic curves over the rationals have mathods has_cm() and cm_discriminant(). We extend these to curves over number fields, and add a function has_rational_cm() to indicate the the curve has CM which is defined over its base field (i.e. the CM discriminant is a square in that field), which is always False over QQ.

Component: elliptic curves

Keywords: complex multiplication

Author: John Cremona

Branch: 4aea367

Reviewer: Marco Streng, Chris Wuthrich

Issue created by migration from https://trac.sagemath.org/ticket/16764

[JohnCremona]

Branch: u/cremona/ticket/16764

[JohnCremona]

Commit: 8dff296

[JohnCremona]

New commits:

552f37a	Work in progress on CM and isogeny classes for EC/NF
5ea36bf	Merge branch 'develop' into isogs-ecnf
8dff296	#16764: CM detection over number fields

[JohnCremona]

comment:2

The changes in isogeny_class.py belong to #16743, not here, and should be ignored. I'll fix this.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

caf41e6

#16764 revert changes to isogeny_class.py which belong in #16743

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 8dff296 to caf41e6

[JohnCremona]

comment:4

Replying to @JohnCremona:

The changes in isogeny_class.py belong to #16743, not here, and should be ignored. I'll fix this.

Done. This probably counts as rewriting history, too bad. #16743 will depend on this anyway.

[fchapoton]

comment:5

Hello,

Lines after a .. NOTE:: should be indented by 4 spaces (not 3).

[JohnCremona]

comment:6

Replying to @fchapoton:

Hello,

Lines after a .. NOTE:: should be indented by 4 spaces (not 3).

Are you sure? See http://sphinx-doc.org/markup/para.html. I was aligning the following text under the n of "note".

[fchapoton]

comment:7

Oh, well. I was trusting

http://www.sagemath.org/doc/developer/coding_basics.html

but I wonder now who is right.

[mstreng]

comment:8

While you're at it, could you change the wording in the first line of the documentation of has_cm for elliptic curves over the rationals to be less ambiguous?

I've seen Complex Multiplication to mean over the base field and to mean over the algebraic closure, so "Returns True iff this elliptic curve has Complex Multiplication." is ambiguous. It could be "Returns True iff this elliptic curve has Complex Multiplication over the algebraic closure." or "Returns True iff this elliptic curve has a CM j-invariant." as in the number field case.

[JohnCremona]

comment:9

Replying to @mstreng:

While you're at it, could you change the wording in the first line of the documentation of has_cm for elliptic curves over the rationals to be less ambiguous?

I've seen Complex Multiplication to mean over the base field and to mean over the algebraic closure, so "Returns True iff this elliptic curve has Complex Multiplication." is ambiguous. It could be "Returns True iff this elliptic curve has Complex Multiplication over the algebraic closure." or "Returns True iff this elliptic curve has a CM j-invariant." as in the number field case.

Good point! Thanks. A suitable change for a reviewer's patch (as they used to be called)?

[JohnCremona]

comment:10

I will follow Marco's suggestion about the documentation for has_cm, and also add a field parameter to has_rational_cm (defaulting to the base_field) as William had suggested.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

8e203af	Merge branch 'develop' into cm
3cacdb2	#16764: adjustments after review

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from caf41e6 to 3cacdb2

[JohnCremona]

comment:12

Done as above; also merged 6.3.rc1 into the branch.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

7852e0f

Merge branch 'develop' into cm

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 3cacdb2 to 7852e0f

[JohnCremona]

comment:15

I hope that merging in the develop branch (as at version 6.4.beta1) will make this easier to review and not harder!

[mstreng]

Reviewer: Marco Streng

[mstreng]

comment:17

The documentation has three references to "the j-invariant of an immaginary quadratic order", which is either a whole Galois orbit or one specific complex number. In all those places it should be "a j-invariant of an imaginary quadratic order". I'm writing a reviewer patch that corrects this.

[mstreng]

Changed branch from u/cremona/ticket/16764 to u/mstreng/ticket/16764

[JohnCremona]

comment:19

Thanks, Marco. If that's the only thing you found I am delighted!

At some point a better implementation of the basic function (detecting whether an algebraic integer is a CM j-invariant) would be good, but the one there is fine for small degrees which is likely to be all that is needed most of the time.

---

New commits:

4aea367

Reviewer patch for ticket 16764 with minor documentation fixes.

[JohnCremona]

Changed commit from 7852e0f to 4aea367

[mstreng]

comment:20

Hi John. If you agree with my reviewer patch, you can set the ticket to postive_review.

---

New commits:

4aea367

Reviewer patch for ticket 16764 with minor documentation fixes.

[mstreng]

comment:21

Oops, I haven't set up my editor properly on this machine. Let me remove the tab character first...

[mstreng]

comment:22

Replying to @mstreng:

Oops, I haven't set up my editor properly on this machine. Let me remove the tab character first...

Never mind, the tab character is in some kind of xcode temp file, not in the actual source code.

[chriswuthrich]

comment:23

All tests pass for me, the documentation builds nicely (I can see a tab character). So this all looks fine to me.

[chriswuthrich]

Changed reviewer from Marco Streng to Marco Streng, Chris Wuthrich

[vbraun]

Changed branch from u/mstreng/ticket/16764 to 4aea367

[jdemeyer]

comment:25

What's the intention of the code

try:
    D = self._CMD
    if D:
        return D
    else:
        raise ValueError("%s does not have CM"%self)
except:
    pass

which is obviously equivalent to

try:
    D = self._CMD
    if D:
        return D
except:
    pass

I assume you meant something like

try:
    D = self._CMD
except AttributeError:
    pass
else:
    if D:
        return D
    else:
        raise ValueError("%s does not have CM"%self)

Apart from this issue, you really should never use except: since that catches also KeyboardInterrupt.

[jdemeyer]

Changed commit from 4aea367 to none

[mstreng]

comment:26

Replying to @jdemeyer:

Apart from this issue, you really should never use except: since that catches also KeyboardInterrupt.

You are right. Could you reopen this ticket? This should not be merged into a stable version without fixing that code.

[jdemeyer]

comment:27

I will fix it in #17076.

[JohnCremona]

comment:28

Apologies, of course I meant to write "except AttributeError". I do know the rules.

To make absolutely sure: the field _CMD stores 0 if it has already been determined that E does not have CM, and in this case the method cm_discriminant raises an error -- not my choice but for compatibility with curves over Q. (I would have preferred to return 0 which is easy for other coed to check).

I'll be happy to review #17076 (though I seem to have forfeited my right to give any code a trustworthy review with this lapse).