#16764 closed enhancement (fixed)
Add CM detection for elliptic curves over number fields
Reported by:  cremona  Owned by:  

Priority:  major  Milestone:  sage6.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 6 years ago by
 Branch set to u/cremona/ticket/16764
 Commit set to 8dff2965d46e6f1e6b25d7b9b950f3d3525a7418
comment:2 followup: ↓ 4 Changed 6 years ago by
The changes in isogeny_class.py belong to #16743, not here, and should be ignored. I'll fix this.
comment:3 Changed 6 years ago by
 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 6 years ago by
 Status changed from new to needs_review
comment:5 followup: ↓ 6 Changed 6 years ago by
Hello,
Lines after a .. NOTE::
should be indented by 4 spaces (not 3).
comment:6 in reply to: ↑ 5 Changed 6 years ago by
Replying to chapoton:
Hello,
Lines after a
.. NOTE::
should be indented by 4 spaces (not 3).
Are you sure? See http://sphinxdoc.org/markup/para.html. I was aligning the following text under the n of "note".
comment:7 Changed 6 years ago by
Oh, well. I was trusting
http://www.sagemath.org/doc/developer/coding_basics.html
but I wonder now who is right.
comment:8 followup: ↓ 9 Changed 6 years ago by
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 jinvariant." as in the number field case.
comment:9 in reply to: ↑ 8 Changed 6 years ago by
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 jinvariant." 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 6 years ago by
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 6 years ago by
 Commit changed from caf41e68e76a753268ce4f4dae36555f0003017b to 3cacdb2fe8a32114e13c26f50c3556800b4668fe
comment:12 Changed 6 years ago by
Done as above; also merged 6.3.rc1 into the branch.
comment:13 Changed 6 years ago by
 Milestone changed from sage6.3 to sage6.4
comment:14 Changed 6 years ago by
 Commit changed from 3cacdb2fe8a32114e13c26f50c3556800b4668fe to 7852e0f54cf117014392095bf623bf76976b4015
Branch pushed to git repo; I updated commit sha1. New commits:
7852e0f  Merge branch 'develop' into cm

comment:15 Changed 6 years ago by
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 6 years ago by
 Reviewers set to Marco Streng
comment:17 Changed 6 years ago by
 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 6 years ago by
 Branch changed from u/cremona/ticket/16764 to u/mstreng/ticket/16764
comment:19 Changed 6 years ago by
 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 jinvariant) 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.

comment:20 Changed 6 years ago by
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.

comment:21 followup: ↓ 22 Changed 6 years ago by
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 6 years ago by
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 6 years ago by
 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 6 years ago by
 Branch changed from u/mstreng/ticket/16764 to 4aea367970e8f902845ed4837a6dd9697619a35e
 Resolution set to fixed
 Status changed from positive_review to closed
comment:25 followup: ↓ 26 Changed 6 years ago by
 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 6 years ago by
Replying to jdemeyer:
Apart from this issue, you really should never use
except:
since that catches alsoKeyboardInterrupt
.
You are right. Could you reopen this ticket? This should not be merged into a stable version without fixing that code.
comment:27 Changed 6 years ago by
I will fix it in #17076.
comment:28 Changed 6 years ago by
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).
New commits:
Work in progress on CM and isogeny classes for EC/NF
Merge branch 'develop' into isogsecnf
#16764: CM detection over number fields