Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#16764 closed enhancement (fixed)

Add CM detection for elliptic curves over number fields

Reported by: cremona Owned by:
Priority: major Milestone: sage-6.4
Component: elliptic curves Keywords: complex multiplication
Cc: Merged in:
Authors: John Cremona Reviewers: Marco Streng, Chris Wuthrich
Report Upstream: N/A Work issues:
Branch: 4aea367 (Commits) Commit:
Dependencies: Stopgaps:

Description

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.

Change History (28)

comment:1 Changed 5 years ago by cremona

  • Branch set to u/cremona/ticket/16764
  • Commit set to 8dff2965d46e6f1e6b25d7b9b950f3d3525a7418

New commits:

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

comment:2 follow-up: Changed 5 years ago by cremona

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

comment:3 Changed 5 years ago by git

  • Commit changed from 8dff2965d46e6f1e6b25d7b9b950f3d3525a7418 to caf41e68e76a753268ce4f4dae36555f0003017b

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

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

comment:4 in reply to: ↑ 2 Changed 5 years ago by cremona

  • Status changed from new to needs_review

Replying to cremona:

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.

comment:5 follow-up: Changed 5 years ago by chapoton

Hello,

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

comment:6 in reply to: ↑ 5 Changed 5 years ago by cremona

Replying to chapoton:

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".

comment:7 Changed 5 years ago by chapoton

Oh, well. I was trusting

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

but I wonder now who is right.

comment:8 follow-up: Changed 5 years ago by 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.

comment:9 in reply to: ↑ 8 Changed 5 years ago by cremona

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)?

comment:10 Changed 5 years ago by cremona

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.

comment:11 Changed 5 years ago by git

  • Commit changed from caf41e68e76a753268ce4f4dae36555f0003017b to 3cacdb2fe8a32114e13c26f50c3556800b4668fe

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

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

comment:12 Changed 5 years ago by cremona

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

comment:13 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:14 Changed 5 years ago by git

  • Commit changed from 3cacdb2fe8a32114e13c26f50c3556800b4668fe to 7852e0f54cf117014392095bf623bf76976b4015

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

7852e0fMerge branch 'develop' into cm

comment:15 Changed 5 years ago by cremona

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

comment:16 Changed 5 years ago by mstreng

  • Reviewers set to Marco Streng

comment:17 Changed 5 years ago by mstreng

  • Created changed from 08/05/14 13:17:45 to 08/05/14 13:17:45
  • Modified changed from 08/29/14 10:19:10 to 08/29/14 10:19:10

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.

comment:18 Changed 5 years ago by mstreng

  • Branch changed from u/cremona/ticket/16764 to u/mstreng/ticket/16764

comment:19 Changed 5 years ago by cremona

  • Commit changed from 7852e0f54cf117014392095bf623bf76976b4015 to 4aea367970e8f902845ed4837a6dd9697619a35e

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:

4aea367Reviewer patch for ticket 16764 with minor documentation fixes.

comment:20 Changed 5 years ago by mstreng

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


New commits:

4aea367Reviewer patch for ticket 16764 with minor documentation fixes.

comment:21 follow-up: Changed 5 years ago by mstreng

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

comment:22 in reply to: ↑ 21 Changed 5 years ago by mstreng

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.

comment:23 Changed 5 years ago by wuthrich

  • Reviewers changed from Marco Streng to Marco Streng, Chris Wuthrich
  • Status changed from needs_review to positive_review

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

comment:24 Changed 5 years ago by vbraun

  • Branch changed from u/mstreng/ticket/16764 to 4aea367970e8f902845ed4837a6dd9697619a35e
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:25 follow-up: Changed 5 years ago by jdemeyer

  • Commit 4aea367970e8f902845ed4837a6dd9697619a35e deleted

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.

comment:26 in reply to: ↑ 25 Changed 5 years ago by mstreng

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.

comment:27 Changed 5 years ago by jdemeyer

I will fix it in #17076.

comment:28 Changed 5 years ago by cremona

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).

Note: See TracTickets for help on using tickets.