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.

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

Improve CM testing for elliptic curves over number fields #35166

Merged
merged 16 commits into from Apr 1, 2023

Conversation

JohnCremona
Copy link
Member

πŸ“š Description

This implements the Cremona-Sutherland algorithm (see :arxiv:2301.11169) for detecting Hilbert Class Polynomials and computing the corresponding CM discriminants. Using this, a new default algorithm for detecting CM j-invariants, and hence whether or not elliptic curves over number fields, is implemented. This is very much faster than the older methods for CM testing which were only fast enough to be reasonable over fields of degree less than 10; the new method works fine up to degree 100 and beyond (as some of the examples show).

Secondly, the caching of the computation of CM orders and discriminants and j-invariants for class numbers up to 100 has been made a lot more efficient through better caching. Before, if you asked for all discriminants with class number (say) 10, it computed all those with class number up to 10 and threw the rest away, so that even if you then asked for class number 9 it had to start from scratch; now that second call will be instantaneous. And If you then ask for class number 11, it will be faster than it was since much of the data needed will have been cached.

This closes #35147

πŸ“ Checklist

  • [x ] I have made sure that the title is self-explanatory and the description concisely explains the PR.
  • [x ] I have linked an issue or discussion.
  • [ x] I have created tests covering the changes.
  • [ x] I have updated the documentation accordingly.

βŒ› Dependencies

None. Based on 10.0.beta0

@JohnCremona
Copy link
Member Author

I see doctest failures and am fixing them.

I suppose the point of having a "draft" PR is that all the automatic tests will run, then one makes it a real PR after fixing issues.

@JohnCremona
Copy link
Member Author

I see doctest failures and am fixing them.

There are no failures when I run the tests locally so am letting the automatic build and test run again.

@JohnCremona
Copy link
Member Author

I see doctest failures and am fixing them.

There are no failures when I run the tests locally so am letting the automatic build and test run again.

OK, there really were, so I have fixed them.

@codecov-commenter
Copy link

codecov-commenter commented Feb 22, 2023

Codecov Report

Patch coverage: 84.91% and project coverage change: -0.04 ⚠️

Comparison is base (c00e6c2) 88.62% compared to head (d83aa3a) 88.58%.

❗ Current head d83aa3a differs from pull request most recent head f9c28ca. Consider uploading reports for the commit f9c28ca to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #35166      +/-   ##
===========================================
- Coverage    88.62%   88.58%   -0.04%     
===========================================
  Files         2148     2140       -8     
  Lines       398855   397570    -1285     
===========================================
- Hits        353480   352192    -1288     
- Misses       45375    45378       +3     
Impacted Files Coverage Ξ”
src/sage/quadratic_forms/special_values.py 96.29% <ΓΈ> (-0.05%) ⬇️
src/sage/schemes/elliptic_curves/cm.py 73.14% <81.57%> (-16.39%) ⬇️
...c/sage/schemes/elliptic_curves/ell_finite_field.py 90.92% <89.23%> (+1.93%) ⬆️
src/sage/quadratic_forms/binary_qf.py 93.28% <100.00%> (-0.02%) ⬇️
src/sage/rings/number_field/number_field.py 93.59% <100.00%> (-0.01%) ⬇️
src/sage/schemes/elliptic_curves/heegner.py 90.96% <100.00%> (-0.01%) ⬇️

... and 152 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

β˜” View full report in Codecov by Sentry.
πŸ“’ Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@roed314 roed314 left a comment

Choose a reason for hiding this comment

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

Generally looks good!

src/sage/schemes/elliptic_curves/ell_finite_field.py Outdated Show resolved Hide resolved
src/sage/schemes/elliptic_curves/ell_finite_field.py Outdated Show resolved Hide resolved
src/sage/schemes/elliptic_curves/ell_finite_field.py Outdated Show resolved Hide resolved
src/sage/schemes/elliptic_curves/ell_finite_field.py Outdated Show resolved Hide resolved
src/sage/schemes/elliptic_curves/cm.py Outdated Show resolved Hide resolved
src/sage/schemes/elliptic_curves/cm.py Outdated Show resolved Hide resolved
src/sage/schemes/elliptic_curves/cm.py Outdated Show resolved Hide resolved
src/sage/schemes/elliptic_curves/cm.py Outdated Show resolved Hide resolved
src/sage/schemes/elliptic_curves/cm.py Outdated Show resolved Hide resolved
src/sage/schemes/elliptic_curves/cm.py Outdated Show resolved Hide resolved
@JohnCremona
Copy link
Member Author

I have now finished my edits in response to the referee

@JohnCremona
Copy link
Member Author

I just fixed a trivial merge conflict which arose because this had been sitting idle for 2 weeks since I addressed all the revier's concerns. Please @roed314 or someone else update the review so that this can be merged!

@github-actions
Copy link

Documentation preview for this PR is ready! πŸŽ‰
Built with commit: f9c28ca

@JohnCremona
Copy link
Member Author

This has now been sitting idle for 4 weeks since I addressed all the revier's concerns. Please @roed314 or someone else update the review so that this can be merged!

@roed314
Copy link
Contributor

roed314 commented Mar 31, 2023

Thanks for the reminder! This looks good to me.

@JohnCremona
Copy link
Member Author

Thanks, @roed314 !

@vbraun vbraun merged commit db7af74 into sagemath:develop Apr 1, 2023
5 checks passed
@mkoeppe mkoeppe added this to the sage-10.0 milestone Apr 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve CM testing for elliptic curves
5 participants