Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

CM elliptic curves missing isogenies #36786

Merged
merged 5 commits into from
Dec 26, 2023
Merged

CM elliptic curves missing isogenies #36786

merged 5 commits into from
Dec 26, 2023

Conversation

JohnCremona
Copy link
Member

Fixes #36780.

For elliptic curves over number fields, with j-invariant 0 and 1728, the function isogeny_degrees_cm was forgetting to multiply a degree bound by 3 or 2 respectively (half the number of units), resulting in some isogenies being missed.

  • [ x] The title is concise, informative, and self-explanatory.
  • [ x] The description explains in detail what this PR is about.
  • [ x] I have linked a relevant issue or discussion.
  • [ x] I have created tests covering the changes.
  • [ x] I have updated the documentation accordingly.

@JohnCremona
Copy link
Member Author

I do not understand what failed or why, and do not know how to find out. Can anyone explain?

@yyyyx4
Copy link
Member

yyyyx4 commented Nov 27, 2023

From here:

sage -t --random-seed=68842648562601485689143741138089216081 src/sage/schemes/elliptic_curves/ell_number_field.py
**********************************************************************
File "src/sage/schemes/elliptic_curves/ell_number_field.py", line 3342, in sage.schemes.elliptic_curves.ell_number_field.EllipticCurve_number_field.?
Failed example:
    E.reducible_primes()
Expected:
    [2, 3, 5]
Got:
    [2, 5]
**********************************************************************
File "src/sage/schemes/elliptic_curves/ell_number_field.py", line 3348, in sage.schemes.elliptic_curves.ell_number_field.EllipticCurve_number_field.?
Failed example:
    E.reducible_primes()
Expected:
    [2, 3, 5, 7, 11, 13]
Got:
    [2, 3]
**********************************************************************
1 item had failures:
   2 of  76 in sage.schemes.elliptic_curves.ell_number_field.EllipticCurve_number_field.?
    [800 tests, 2 failures, 44.19 s]

@JohnCremona
Copy link
Member Author

Thanks. My 3rd commit should have fixed those

@JohnCremona
Copy link
Member Author

As far as I can see, the test failures have nothing to do with my changes.

@JohnCremona
Copy link
Member Author

I looked ta the failing tests and as far as I can see they have nothing to do with any code changed in my PR.

@github-actions GitHub Actions
Copy link

Documentation preview for this PR (built with commit 7621f8e; changes) is ready! 🎉

@JohnCremona
Copy link
Member Author

After updating (merge with develop) there are still test failures which have nothing to do with me (the test platform bailed out after not finding mathjax).

@dimpase
Copy link
Member

dimpase commented Dec 19, 2023

You can click on "Details" button of the failing test, and check that tests which fail aren't related to your ticket.

@JohnCremona JohnCremona requested a review from dimpase December 19, 2023 14:49
@JohnCremona
Copy link
Member Author

You can click on "Details" button of the failing test, and check that tests which fail aren't related to your ticket.

Yes, I have been doing that for three weeks, after which nothing changes. Now I cannot even remember what the bug was.

fchapoton
fchapoton approved these changes Dec 19, 2023
View reviewed changes
Copy link
Contributor

@fchapoton fchapoton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, looks good enough

vbraun pushed a commit to vbraun/sage that referenced this pull request Dec 21, 2023
5b3e255 
sagemathgh-36786: CM elliptic curves missing isogenies
    
Fixes sagemath#36780.

For elliptic curves over number fields, with j-invariant 0 and 1728, the
function isogeny_degrees_cm was forgetting to multiply a degree bound by
3 or 2 respectively (half the number of units), resulting in some
isogenies being missed.


- [ x] The title is concise, informative, and self-explanatory.
- [ x] The description explains in detail what this PR is about.
- [ x] I have linked a relevant issue or discussion.
- [ x] I have created tests covering the changes.
- [ x] I have updated the documentation accordingly.
    
URL: sagemath#36786
Reported by: John Cremona
Reviewer(s): Frédéric Chapoton
vbraun pushed a commit to vbraun/sage that referenced this pull request Dec 22, 2023
928cacc 
sagemathgh-36786: CM elliptic curves missing isogenies
    
Fixes sagemath#36780.

For elliptic curves over number fields, with j-invariant 0 and 1728, the
function isogeny_degrees_cm was forgetting to multiply a degree bound by
3 or 2 respectively (half the number of units), resulting in some
isogenies being missed.


- [ x] The title is concise, informative, and self-explanatory.
- [ x] The description explains in detail what this PR is about.
- [ x] I have linked a relevant issue or discussion.
- [ x] I have created tests covering the changes.
- [ x] I have updated the documentation accordingly.
    
URL: sagemath#36786
Reported by: John Cremona
Reviewer(s): Frédéric Chapoton
vbraun pushed a commit to vbraun/sage that referenced this pull request Dec 23, 2023
fe9c9a6 
sagemathgh-36786: CM elliptic curves missing isogenies
    
Fixes sagemath#36780.

For elliptic curves over number fields, with j-invariant 0 and 1728, the
function isogeny_degrees_cm was forgetting to multiply a degree bound by
3 or 2 respectively (half the number of units), resulting in some
isogenies being missed.


- [ x] The title is concise, informative, and self-explanatory.
- [ x] The description explains in detail what this PR is about.
- [ x] I have linked a relevant issue or discussion.
- [ x] I have created tests covering the changes.
- [ x] I have updated the documentation accordingly.
    
URL: sagemath#36786
Reported by: John Cremona
Reviewer(s): Frédéric Chapoton
vbraun pushed a commit to vbraun/sage that referenced this pull request Dec 24, 2023
023fdae 
sagemathgh-36786: CM elliptic curves missing isogenies
    
Fixes sagemath#36780.

For elliptic curves over number fields, with j-invariant 0 and 1728, the
function isogeny_degrees_cm was forgetting to multiply a degree bound by
3 or 2 respectively (half the number of units), resulting in some
isogenies being missed.


- [ x] The title is concise, informative, and self-explanatory.
- [ x] The description explains in detail what this PR is about.
- [ x] I have linked a relevant issue or discussion.
- [ x] I have created tests covering the changes.
- [ x] I have updated the documentation accordingly.
    
URL: sagemath#36786
Reported by: John Cremona
Reviewer(s): Frédéric Chapoton
@vbraun vbraun merged commit a46575d into sagemath:develop Dec 26, 2023
@mkoeppe mkoeppe added this to the sage-10.3 milestone Dec 26, 2023
@JohnCremona JohnCremona mentioned this pull request May 1, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fchapoton fchapoton fchapoton approved these changes

@dimpase dimpase Awaiting requested review from dimpase

Assignees
No one assigned
Labels
c: elliptic curves
Projects
None yet
Milestone
sage-10.3
Development

Successfully merging this pull request may close these issues.

Elliptic curves with CM over number fields fail to find all isogenies
6 participants
@JohnCremona @yyyyx4 @dimpase @fchapoton @vbraun @mkoeppe