Implement modularity test for elliptic curves #34989

JohnCremona · 2023-02-06T17:05:50Z

… fields

…adratic fields

yyyyx4 · 2023-02-06T17:22:00Z

You can use one of a few select keywords to link a pull request to an issue. For example, editing the description to include "fixes #22697" will instruct GitHub to automatically close #22697 when this pull request gets merged.

JohnCremona · 2023-02-06T17:24:13Z

Thanks for adding the label. I could not. It's an enhancement, not a bug fix.

codecov-commenter · 2023-02-08T10:08:09Z

Codecov Report

Base: 88.59% // Head: 88.59% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (d369bd3) compared to base (104dde9).
Patch coverage: 96.26% of modified lines in pull request are covered.

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

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #34989      +/-   ##
===========================================
- Coverage    88.59%   88.59%   -0.01%     
===========================================
  Files         2136     2140       +4     
  Lines       396142   397131     +989     
===========================================
+ Hits        350948   351819     +871     
- Misses       45194    45312     +118

Impacted Files	Coverage Δ
src/sage/algebras/free_algebra_element.py	`82.50% <ø> (ø)`
src/sage/algebras/fusion_rings/fusion_ring.py	`93.93% <ø> (ø)`
src/sage/algebras/lie_algebras/free_lie_algebra.py	`95.43% <ø> (ø)`
src/sage/algebras/lie_algebras/lie_algebra.py	`91.10% <ø> (ø)`
src/sage/algebras/lie_algebras/morphism.py	`90.07% <ø> (ø)`
src/sage/algebras/quatalg/quaternion_algebra.py	`92.68% <ø> (ø)`
src/sage/arith/misc.py	`90.99% <ø> (ø)`
src/sage/categories/category_with_axiom.py	`98.71% <ø> (ø)`
src/sage/categories/coxeter_groups.py	`94.59% <ø> (ø)`
...age/categories/finite_complex_reflection_groups.py	`79.12% <ø> (ø)`
... and 165 more

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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

JohnCremona · 2023-02-08T10:08:17Z

I fixed the two minor lint failures (not in my code!)

JohnCremona · 2023-02-08T10:11:42Z

...and then I merged the latest develop and fixed a merge conflict

tscrim

I didn't check the mathematics behind it as it is too far outside my expertise. However, the code has been reviewed.

tscrim · 2023-02-10T02:12:11Z

src/sage/schemes/elliptic_curves/ell_number_field.py

-r"""
-Elliptic curves over number fields
+r"""Elliptic curves over number fields


I would revert this as our convention is one-line on a new line from the """.

Will do. My emacs python mode always does this, I wish it would not!

tscrim · 2023-02-10T02:13:21Z

src/sage/schemes/elliptic_curves/ell_number_field.py

+        r"""Returns `True` if the base field is totally real and modularity of
+        this curve can be proved, otherwise `False`.
+
+        INPUT:
+
+        - ``verbose`` (bool, default ``False``) -- if True, outputs a reason for the conclusion.


Suggested change

r"""Returns `True` if the base field is totally real and modularity of

this curve can be proved, otherwise `False`.

INPUT:

- ``verbose`` (bool, default ``False``) -- if True, outputs a reason for the conclusion.

r"""

Return ``True`` if the base field is totally real and modularity of

this curve can be proved, otherwise ``False``.

INPUT:

- ``verbose`` -- boolean (default: ``False``); if ``True``, outputs a reason for the conclusion

tscrim · 2023-02-10T02:13:53Z

src/sage/schemes/elliptic_curves/ell_number_field.py

+            When `False` is returned, it does not mean that the curve
+            is not modular!  Only that modularity cannot be proved
+            with current knowledge and implemented methods.  It is
+            expected that the return value will be `True` for all
+            curves defined over totally real fields, and for all
+            defined over imaginary quadratic fields over which the
+            modular curve $X_0(15)$ (with label `15a1`) has rank
+            zero.  Any curve defined over totally real fields for
+            which the return value is `False` is of interest, as its
+            mod `p` Galois representations for the primes 3, 5 and 7
+            are simultaneously nonmaximal.


Suggested change

When `False` is returned, it does not mean that the curve

is not modular! Only that modularity cannot be proved

with current knowledge and implemented methods. It is

expected that the return value will be `True` for all

curves defined over totally real fields, and for all

defined over imaginary quadratic fields over which the

modular curve $X_0(15)$ (with label `15a1`) has rank

zero. Any curve defined over totally real fields for

which the return value is `False` is of interest, as its

mod `p` Galois representations for the primes 3, 5 and 7

are simultaneously nonmaximal.

When ``False`` is returned, it does not mean that the curve

is not modular! Only that modularity cannot be proved

with current knowledge and implemented methods. It is

expected that the return value will be ``True`` for all

curves defined over totally real fields, and for all

defined over imaginary quadratic fields over which the

modular curve `X_0(15)` (with label `15a1`) has rank

zero. Any curve defined over totally real fields for

which the return value is ``False`` is of interest, as its

mod `p` Galois representations for the primes 3, 5 and 7

are simultaneously nonmaximal.

tscrim · 2023-02-10T02:14:14Z

src/sage/schemes/elliptic_curves/ell_number_field.py

+        real case, and relies on theorems in the following papers:
+        [Chen1996]_, [FLHS2015]_, [Thorne2016]_, [CarNew2023]_.
+
+        EXAMPLES.  Set ``verbose=True`` to see a reason for the conclusion::


Suggested change

EXAMPLES. Set ``verbose=True`` to see a reason for the conclusion::

EXAMPLES:

Set ``verbose=True`` to see a reason for the conclusion::

tscrim · 2023-02-10T02:14:47Z

src/sage/schemes/elliptic_curves/ell_number_field.py

+
+        """
+        def not_from(group, p, j):
+            return len(_modular_polynomial(group, p, j).roots()) == 0


Suggested change

return len(_modular_polynomial(group, p, j).roots()) == 0

return not _modular_polynomial(group, p, j).roots()

This should be slightly faster.

tscrim · 2023-02-10T02:15:18Z

src/sage/schemes/elliptic_curves/ell_number_field.py

+        return False # We've run out of tricks!
+
+def _modular_polynomial(group, p, j):
+    r"""Helper function for the method :meth:`is_modular`.


Suggested change

r"""Helper function for the method :meth:`is_modular`.

r"""

Helper function for the method :meth:`is_modular`.

tscrim · 2023-02-10T02:16:05Z

src/sage/schemes/elliptic_curves/ell_number_field.py

+    - ``group`` (string) -- one of 'borel', 'split', 'nonsplit'.
+
+    - ``p`` (int) -- a prime number, either 3 or 5 or 7.
+
+    - ``j`` -- an(algebraic number, the `j`-invariant of an elliptic curve.


Suggested change

- ``group`` (string) -- one of 'borel', 'split', 'nonsplit'.

- ``p`` (int) -- a prime number, either 3 or 5 or 7.

- ``j`` -- an(algebraic number, the `j`-invariant of an elliptic curve.

- ``group`` -- string; one of ``'borel'``, ``'split'``, ``'nonsplit'``

- ``p`` -- integer; a prime number, either 3 or 5 or 7

- ``j`` -- an algebraic number; the `j`-invariant of an elliptic curve

tscrim · 2023-02-10T02:19:56Z

src/sage/schemes/elliptic_curves/ell_number_field.py

+        sage: _modular_polynomial('split', 5, j)
+        x^15 + 30*x^14 + 390*x^13 + 2800*x^12 + 11175*x^11 + 2658577186/161051*x^10 - 8545073600/161051*x^9 - 3318236600/14641*x^8 + 79768901125/161051*x^7 + 950952612750/161051*x^6 + 3261066516250/161051*x^5 + 5917349970000/161051*x^4 + 5381326371875/161051*x^3 + 45039331250/14641*x^2 - 3377861937500/161051*x - 2135097075000/161051
+        sage: _modular_polynomial('nonsplit', 5, j)
+        43605793936/161051*x^10 + 609382899680/161051*x^9 + 3805439994680/161051*x^8 + 13957069930640/161051*x^7 + 33226111304085/161051*x^6 + 4866186482686/14641*x^5 + 58961494233415/161051*x^4 + 43734191948140/161051*x^3 + 20843516212195/161051*x^2 + 5741876955930/161051*x + 690283481689/161051
+        sage: _modular_polynomial('borel', 7, j)
+        x^8 + 28*x^7 + 322*x^6 + 1904*x^5 + 5915*x^4 + 8624*x^3 + 4018*x^2 + 242490084/161051*x + 49
+        sage: _modular_polynomial('split', 7, j)
+        x^28 - 42*x^27 + 819*x^26 - 9842*x^25 + 81543*x^24 - 493416*x^23 + 2251424*x^22 - 1268191320692/161051*x^21 + 3410611518671/161051*x^20 - 6947130862854/161051*x^19 + 10098387132407/161051*x^18 - 8011521604618/161051*x^17 - 509395128662/14641*x^16 + 32582181402218/161051*x^15 - 62691210143713/161051*x^14 + 73259301822126/161051*x^13 - 46379791168053/161051*x^12 - 6277600417172/161051*x^11 + 46742832789352/161051*x^10 - 49641085864608/161051*x^9 + 2343504014803/14641*x^8 - 861578737314/161051*x^7 - 12167703919007/161051*x^6 + 14278547318070/161051*x^5 - 9423834664881/161051*x^4 + 3112085963896/161051*x^3 - 457632005824/161051*x^2 + 30845635072/161051*x + 122023936/161051
+        sage: _modular_polynomial('nonsplit', 7, j)
+        400320064/161051*x^21 + 13029623152/161051*x^20 + 228088753900/161051*x^19 + 2408913460821/161051*x^18 + 16642817648786/161051*x^17 + 80889087315407/161051*x^16 + 291040779803200/161051*x^15 + 801917686416123/161051*x^14 + 1729396406547138/161051*x^13 + 2959141014912537/161051*x^12 + 4049663724749832/161051*x^11 + 4450611572214360/161051*x^10 + 3931865527421272/161051*x^9 + 2786855392905248/161051*x^8 + 1576387623537152/161051*x^7 + 704811439294144/161051*x^6 + 22297037611520/14641*x^5 + 5896572491264/14641*x^4 + 12558313746944/161051*x^3 + 152158103552/14641*x^2 + 136821293056/161051*x + 5155295232/161051


Suggested change

sage: _modular_polynomial('split', 5, j)

x^15 + 30*x^14 + 390*x^13 + 2800*x^12 + 11175*x^11 + 2658577186/161051*x^10 - 8545073600/161051*x^9 - 3318236600/14641*x^8 + 79768901125/161051*x^7 + 950952612750/161051*x^6 + 3261066516250/161051*x^5 + 5917349970000/161051*x^4 + 5381326371875/161051*x^3 + 45039331250/14641*x^2 - 3377861937500/161051*x - 2135097075000/161051

sage: _modular_polynomial('nonsplit', 5, j)

43605793936/161051*x^10 + 609382899680/161051*x^9 + 3805439994680/161051*x^8 + 13957069930640/161051*x^7 + 33226111304085/161051*x^6 + 4866186482686/14641*x^5 + 58961494233415/161051*x^4 + 43734191948140/161051*x^3 + 20843516212195/161051*x^2 + 5741876955930/161051*x + 690283481689/161051

sage: _modular_polynomial('borel', 7, j)

x^8 + 28*x^7 + 322*x^6 + 1904*x^5 + 5915*x^4 + 8624*x^3 + 4018*x^2 + 242490084/161051*x + 49

sage: _modular_polynomial('split', 7, j)

x^28 - 42*x^27 + 819*x^26 - 9842*x^25 + 81543*x^24 - 493416*x^23 + 2251424*x^22 - 1268191320692/161051*x^21 + 3410611518671/161051*x^20 - 6947130862854/161051*x^19 + 10098387132407/161051*x^18 - 8011521604618/161051*x^17 - 509395128662/14641*x^16 + 32582181402218/161051*x^15 - 62691210143713/161051*x^14 + 73259301822126/161051*x^13 - 46379791168053/161051*x^12 - 6277600417172/161051*x^11 + 46742832789352/161051*x^10 - 49641085864608/161051*x^9 + 2343504014803/14641*x^8 - 861578737314/161051*x^7 - 12167703919007/161051*x^6 + 14278547318070/161051*x^5 - 9423834664881/161051*x^4 + 3112085963896/161051*x^3 - 457632005824/161051*x^2 + 30845635072/161051*x + 122023936/161051

sage: _modular_polynomial('nonsplit', 7, j)

400320064/161051*x^21 + 13029623152/161051*x^20 + 228088753900/161051*x^19 + 2408913460821/161051*x^18 + 16642817648786/161051*x^17 + 80889087315407/161051*x^16 + 291040779803200/161051*x^15 + 801917686416123/161051*x^14 + 1729396406547138/161051*x^13 + 2959141014912537/161051*x^12 + 4049663724749832/161051*x^11 + 4450611572214360/161051*x^10 + 3931865527421272/161051*x^9 + 2786855392905248/161051*x^8 + 1576387623537152/161051*x^7 + 704811439294144/161051*x^6 + 22297037611520/14641*x^5 + 5896572491264/14641*x^4 + 12558313746944/161051*x^3 + 152158103552/14641*x^2 + 136821293056/161051*x + 5155295232/161051

sage: _modular_polynomial('split', 5, j)

x^15 + 30*x^14 + 390*x^13 + 2800*x^12 + 11175*x^11 + 2658577186/161051*x^10

- 8545073600/161051*x^9 - 3318236600/14641*x^8 + 79768901125/161051*x^7 + 950952612750/161051*x^6

+ 3261066516250/161051*x^5 + 5917349970000/161051*x^4 + 5381326371875/161051*x^3

+ 45039331250/14641*x^2 - 3377861937500/161051*x - 2135097075000/161051

sage: _modular_polynomial('nonsplit', 5, j)

43605793936/161051*x^10 + 609382899680/161051*x^9 + 3805439994680/161051*x^8 + 13957069930640/161051*x^7

+ 33226111304085/161051*x^6 + 4866186482686/14641*x^5 + 58961494233415/161051*x^4

+ 43734191948140/161051*x^3 + 20843516212195/161051*x^2 + 5741876955930/161051*x + 690283481689/161051

sage: _modular_polynomial('borel', 7, j)

x^8 + 28*x^7 + 322*x^6 + 1904*x^5 + 5915*x^4 + 8624*x^3 + 4018*x^2 + 242490084/161051*x + 49

sage: _modular_polynomial('split', 7, j)

x^28 - 42*x^27 + 819*x^26 - 9842*x^25 + 81543*x^24 - 493416*x^23 + 2251424*x^22

- 1268191320692/161051*x^21 + 3410611518671/161051*x^20 - 6947130862854/161051*x^19

+ 10098387132407/161051*x^18 - 8011521604618/161051*x^17 - 509395128662/14641*x^16

+ 32582181402218/161051*x^15 - 62691210143713/161051*x^14 + 73259301822126/161051*x^13

- 46379791168053/161051*x^12 - 6277600417172/161051*x^11 + 46742832789352/161051*x^10

- 49641085864608/161051*x^9 + 2343504014803/14641*x^8 - 861578737314/161051*x^7

- 12167703919007/161051*x^6 + 14278547318070/161051*x^5 - 9423834664881/161051*x^4

+ 3112085963896/161051*x^3 - 457632005824/161051*x^2 + 30845635072/161051*x + 122023936/161051

sage: _modular_polynomial('nonsplit', 7, j)

400320064/161051*x^21 + 13029623152/161051*x^20 + 228088753900/161051*x^19

+ 2408913460821/161051*x^18 + 16642817648786/161051*x^17 + 80889087315407/161051*x^16

+ 291040779803200/161051*x^15 + 801917686416123/161051*x^14 + 1729396406547138/161051*x^13

+ 2959141014912537/161051*x^12 + 4049663724749832/161051*x^11 + 4450611572214360/161051*x^10

+ 3931865527421272/161051*x^9 + 2786855392905248/161051*x^8 + 1576387623537152/161051*x^7

+ 704811439294144/161051*x^6 + 22297037611520/14641*x^5 + 5896572491264/14641*x^4

+ 12558313746944/161051*x^3 + 152158103552/14641*x^2 + 136821293056/161051*x + 5155295232/161051

Splitting up some long lines. Since this @#$% editor window is so small, I am not sure that it is cut around 80 char/line. It can be a bit longer, but they need a few cuts.

OK -- I willl make all the changes suggested an update my branch. Thanks!

Done -- I hope I did not miss anything. If you are happy @tscrim could you click on the Resolve Conversation button -- I assume that is what it is for.

tscrim · 2023-02-11T12:26:48Z

@JohnCremona Thank you. I am happy with the doc. As far as I can tell the code does what it says it does, but I can't say that with any sort of confidence. If nobody can verify the math in a week or so, just ping me again (if I forget about this) and I will positively review this.

fchapoton · 2023-02-17T20:38:17Z

src/sage/schemes/elliptic_curves/ell_number_field.py

+        if p == 7:
+            return ((4*x**2+5*x+2)*(x**2+3*x+4)*(x**2+10*x+4)*(3*x+1))**3-j*(x**3+x**2-2*x-1)**7
+
+    raise RuntimeError # cannot happen -- the assertion would have failed.


so this line is useless and should be removed

I find there is some utility in this as it would catch bugs (or at least make it easier to see where they are coming from) by raising a specific type of exception rather that silently returning None.

but this line cannot be reached at all, I think

That's the point. It should never be reached. I am treating it like an assert statement; something that should never be false/reached. Actually, would that be okay with you: assert False?

I am sorry, but I do not see the point. We are first checking the input parameters to belong to an explicit list. Then, for each member of the list, the code path ends by a return. So this proves that the final line can never happen.

You're exactly right that this code is currently bug-free. Yet, suppose I change this method, but I introduce a bug for some case. Then I run this code, and without this raise, the code silently returns a wrong answer of None. Now this is highly likely to fail, but somewhere else. However, the failure is not guaranteed (no reasonable person should trap a RuntimeError or an AssertionError), which means a wrong answer could actually be returned. Thus, this one extra line makes the code more robust against future errors that could be introduced.

ok, ok. I do not agree, but will not oppose any longer, for doc and code inside a hidden method. Good for me

fchapoton · 2023-02-17T20:39:07Z

src/sage/schemes/elliptic_curves/ell_number_field.py

+
+        INPUT:
+
+        - ``verbose`` (bool, default ``False``) -- if True, outputs a reason for the conclusion.


please write "if True"

fchapoton · 2023-02-17T20:39:58Z

src/sage/schemes/elliptic_curves/ell_number_field.py

+        Traceback (most recent call last):
+        ...
+        ValueError: (group, p) must be one of ...
+        sage: _modular_polynomial('borel', 101, j)


one test would be enough

This is testing a different type of failure, that group is correct but not one of the correct primes. So I find some utility in this test.

but this is running through the same code path, no ?

Sort of. However, it is passing an equality of one part of the pair, but not the second. Putting aside the pedagogical aspects, I am treating this block of code like

if group in [A,B,C]: if p in [X, Y, Z]: # Although this depends on if A, B, or C

But this is not how the code is written, so I persist to say that could just keep one test here.

No, but it demonstrates what the behavior should be: there is a dependency on p as well. I don't see the harm in another test being there.

idem, if you insist, I will not oppose

fchapoton · 2023-02-17T20:40:24Z

src/sage/schemes/elliptic_curves/ell_number_field.py

+                print("Modularity not established: this curve has small image at 3 and 5")
+
+        return False # We've run out of tricks!
+


two blank lines here (pep8)

JohnCremona · 2023-02-19T17:33:44Z

Thanks to both @fchapoton anf @tscrim for taking their time with this. I'm sorry that my little utility function caused so much not very fruitful discussion!

I have an alternative suggestion (at the risk of you both cursing me). What the code does is to test that the elliptic curve has certain properties at some of all of the primes 3, 5, 7. The properties concern the mod-p Galois representation attached the curve, specifically whether the projective image of the representation is or is not contained in one of the standard subgroups of the codomain (which is the group PGL(2,p)): Borel, split Cartan etc. In each case the condition can be tested by checking whether or not a certain polynomial whose coefficients depend on the curve (just its j-invariant) have a root over the field of definition of the curve. For all the cases needed here, the corresponding modular curve has genus 0 (it is P^1) so one is simply testing if some rational function with coefficients in QQ takes j as a value.

Anyway, it would be neater to implement this tests via methods of the class GaloisRepresentation defined in gal_reps_number_field. The constructor for that does no work at all, and I would add methods with names like "is_Borel(p)" which would return True/False accordingly, for certain primes p (and calling the method on other primes would raise a NotImplementedError). I would do this not just for the primes and subgroups needed for this application but for some more, and that would be more useful.

I will do this refactoring in the next day or two; and I will also take on board @fchapoton 's usual useful points about my docstring hygiene.

Hence I'll change the tag on this to "needs work" until I have updated the PR.

…697modularity

JohnCremona · 2023-02-20T19:10:27Z

I have done what I said above. I'll set the PR back to "needs review" once the automatic tests have run and passed (or I have fixed any issues arising).

fchapoton · 2023-02-20T19:16:54Z

src/sage/schemes/elliptic_curves/ell_number_field.py

            there exist infinitely many elliptic curves (even up to
            twist) whose mod 3 and mod 5 Galois representations are
-            both reducible (in other words, which posess rational
+            both reducible (in other words, which posess a rational


possess (with more s) ?

fchapoton · 2023-02-20T19:19:18Z

src/sage/schemes/elliptic_curves/gal_reps_number_field.py

+            return (j-1728).is_square()
+
+        raise NotImplementedError("Only implemented for p==2")
+


Error messages should rather start with a smallcap letter, i.e. not with a capital. This is python convention, don't ask me why..

OK, I am editing now, how will I know you have finished?

OK, I am editing now, how will I know you have finished?

These are submitted in a batch AFAIK. So I think @fchapoton finished looking at things.

yes, sorry, busy with other matters. I have no other comment

yes, sorry, busy with other matters. I have no other comment

thanks -- as always -- for your attention to detail

tscrim · 2023-02-24T05:49:09Z

Then let's get this in. Thank you.

JohnCremona · 2023-02-24T11:40:36Z

I have added references to two more cases recently proved: all curves over totally real cubic fields and t.r. quartic fields not containing sqrt(5).

Also, while I appreciate the code reviews by @tscrim and @fchapoton but I really want someone who knows the field to look at this so I am setting it back to needs review (and will email come people). If I am wrong about the expertise of either existing reviewer, I apologise!

tornaria

I'm yet to look carefully at the galreps new methods. Perhaps if you have a specific reference for each test it would help me double check.

tornaria · 2023-02-26T14:16:56Z

src/sage/schemes/elliptic_curves/ell_number_field.py

+not exist a global minimal equation over `K`, when `K` does not have
+class number one.  When a minimal model does exist the method
+:meth:`global_minimal_model()` will compute it, and otherwise compute
+a model which is miniaml at all primes except one.  Another difference


Suggested change

a model which is miniaml at all primes except one. Another difference

a model which is minimal at all primes except one. Another difference

tornaria · 2023-02-26T14:37:06Z

src/sage/schemes/elliptic_curves/ell_number_field.py

@@ -4067,3 +4073,183 @@ def rational_points(self, **kwds):
            E = E.change_ring(kwds['F'])

        return [E(pt) for pt in Curve(self).rational_points(**kwds)]
+
+    def is_modular(self, verbose=False):


I wonder if this is the right name for the function, since E.is_modular() == False does not mean E is not modular, only that this code doesn't know how to prove it.

Perhaps: is_known_modular? is_provable_modular?

Alternatively, maybe is_modular() could return None instead of False when it doesn't know? So:

True: proved modular

False: proved non-modular (??)

None: unknown to current code

I'm just thinking out loud; I don't know what is done in other similar cases (are there?).

I think this needs wider discussion, I will leave it for now

Perhaps raising a NotImplementedError, with a suitable comment, instead of returning False?

After talking to a few people at COUNT I have a different suggestion: call the method prove_modularity, to be similar to E.prove_BSD(). I'll look at this furthher on Tuesday.

tornaria · 2023-02-26T14:52:58Z

src/sage/schemes/elliptic_curves/gal_reps_number_field.py

+            True
+            sage: E = EllipticCurve('121a1')
+            sage: G = GaloisRepresentation(E)
+            sage: G.is_borel(11)


Suggested change

sage: G.is_borel(11)

sage: G.is_borel(11) # long time

On a fast machine (i7-9700 CPU @ 3.00GHz):

Warning, slow doctest: G.is_borel(11) Test ran for 1.57 s, check ran for 0.00 s

OK (this one is longer since X_0(11) has genus >0)

tornaria · 2023-02-26T15:00:10Z

src/sage/schemes/elliptic_curves/ell_number_field.py

+            sage: E.is_modular(verbose=True)
+            Modularity not established: this curve has small image at 3 and 5
+            False
+            sage: EllipticCurve('15a1').change_ring(K).rank()


Suggested change

sage: EllipticCurve('15a1').change_ring(K).rank()

sage: EllipticCurve('15a1').change_ring(K).rank() # long time

On a fast machine (i7-9700 CPU @ 3.00GHz):

Warning, slow doctest: EllipticCurve('15a1').change_ring(K).rank() Test ran for 0.72 s, check ran for 0.00 s

This is borderline (0.72s is less than 1s, although on a slower/server machine/CI it will be > 1s). But since this is not really a doctest to the current method, I don't see any harm in marking it long time.

OK, but I did not know that 1s was the criterion for long time!

tornaria · 2023-02-26T15:05:31Z

src/doc/en/reference/references/index.rst

+.. [Thorne2016] Jack Thorne.
+                *Automorphy of some residually dihedral Galois representations.*
+                Mathematische Annalen 364 (2016), No. 1--2, pp. 589--648.
+
 .. [Tho2011] Anders Thorup. *ON THE INVARIANTS OF THE SPLITTING ALGEBRA*,
             2011, :arxiv:`1105.4478`



I noted Wiles, etc. are missing. Maybe they could be added as a reference for All elliptic curves over QQ are modular?

OK, will do

I added two references for the Q case, and expanded the ALGORITHM section to be much more explicit, explaining the exact checks carried out. So the extra information is in the docstring, not in the message output with verbose=True. Is That OK?

tornaria · 2023-02-26T15:06:43Z

src/sage/schemes/elliptic_curves/ell_number_field.py

+
+        if d == 1: # base field QQ
+            if verbose:
+                print("All elliptic curves over QQ are modular")


Perhaps add here references to theorems that prove the fact used in each case?

Could be in a comment, or maybe nicer in the string printed in the verbose case.

See previous reply. It would not be possible to give the citations in the verbose message without duplicating the citations I already put in, which would be very tedious to do. Perhaps it is possible to give a URL for the official docstring in the reference manual on the sagemath website.

tornaria · 2023-02-26T15:22:59Z

src/sage/schemes/elliptic_curves/gal_reps_number_field.py

+        try:
+            p = ZZ(p)
+        except TypeError:
+            raise ValueError("p = {} should be a prime integer".format(p))


Suggested change

raise ValueError("p = {} should be a prime integer".format(p))

raise ValueError(f"{p = } should be a prime integer")

Do you know about f-strings? They are very useful (https://docs.python.org/3/tutorial/inputoutput.html#tut-f-strings)

I will look these up and use them in future, but do not feel inclined to change all these strings now.

JohnCremona · 2023-02-27T13:02:39Z

Thanks for the review @tornaria ! I will follow your suggestions soon (at the COUNT conference)

JohnCremona · 2023-02-28T20:35:24Z

I converted this to draft since it is likely that I will not finish with working on it until next week.

github-actions · 2023-11-15T11:52:54Z

Documentation preview for this PR (built with commit 03a0823; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

fchapoton · 2024-09-26T06:18:18Z

hello ? any hope to move forward here ?

JohnCremona · 2024-09-26T09:26:34Z

hello ? any hope to move forward here ?

Sorry, I had forgotten about this. I got stuck trying to work out exactly under what conditions published results allow concluding with essentially no work, or with only the work I had already implemented. (For example, if you take a curve defined over Q it is certainly modular as a curve defined over Q but not, I think, aafter an abitrary base change -- there are results on "solvable base change" but not all extensions are solvable.

I will try to revert to something fairly simple which does not try to be best possible.

JohnCremona · 2025-01-31T10:04:11Z

Thanks for the reminder, I will move this up my list of things to do. Classic case of something reasonable being delayed by the desire to do something better...

JohnCremona added 2 commits February 3, 2023 15:04

sagemath#22697: modularity test for elliptic curves over totally real…

37c12b6

… fields

sagemath#22697: modularity test for elliptic curves over imaginary qu…

a752d28

…adratic fields

JohnCremona mentioned this pull request Feb 6, 2023

Fix doctest in sage/schemes/elliptic_curves/ell_number_field.py #34959

Closed

fchapoton added the c: elliptic curves label Feb 6, 2023

JohnCremona changed the title ~~Addresses Issue #22697 (modularity of elliptic curves)~~ fixes issue #22697 (modularity of elliptic curves) Feb 6, 2023

JohnCremona changed the title ~~fixes issue #22697 (modularity of elliptic curves)~~ Closes issue #22697 (modularity of elliptic curves) Feb 6, 2023

fchapoton added the t: enhancement label Feb 6, 2023

kwankyu added the p: major / 3 label Feb 7, 2023

JohnCremona requested review from mstreng and williamstein February 7, 2023 09:40

fix two lint issues

9dee295

JohnCremona added 2 commits February 8, 2023 10:08

Merge branch 'develop' into 22697modularity

5d629bc

Merge remote-tracking branch 'upstream/develop' into 22697modularity

01cbca9

JohnCremona changed the title ~~Closes issue #22697 (modularity of elliptic curves)~~ Implement modularity test for elliptic curves Feb 8, 2023

JohnCremona added 2 commits February 8, 2023 10:52

Merge branch 'develop' into 22697modularity

844c1e8

Merge branch 'sagemath:develop' into 22697modularity

b159ce7

tscrim reviewed Feb 10, 2023

View reviewed changes

sagemath#22697 changes suggested by reviewer

d5e0e30

tscrim requested a review from tornaria February 11, 2023 12:32

fchapoton reviewed Feb 17, 2023

View reviewed changes

Merge branch '22697modularity' of github.com:JohnCremona/sage into 22…

0aeaa66

…697modularity

fchapoton reviewed Feb 20, 2023

View reviewed changes

JohnCremona added 2 commits February 20, 2023 19:41

small fixes after review

f891f77

fix docstring typos

d369bd3

JohnCremona added s: needs review and removed s: needs work labels Feb 23, 2023

tscrim approved these changes Feb 24, 2023

View reviewed changes

tscrim added s: positive review and removed s: needs review labels Feb 24, 2023

added the t.r. cubic and quartic cases

22f547b

JohnCremona added s: needs review and removed s: positive review labels Feb 24, 2023

tornaria reviewed Feb 26, 2023

View reviewed changes

Merge remote-tracking branch 'upstream/develop' into 22697modularity

9afba37

JohnCremona marked this pull request as draft February 28, 2023 20:34

vbraun force-pushed the develop branch from 883e05f to e349b00 Compare November 12, 2023 16:25

JohnCremona added 2 commits November 14, 2023 11:40

sagemath#22697 modularity: work in progress

d09e52a

Merge remote-tracking branch 'upstream/develop' into 22697modularity

2a0429e

vbraun force-pushed the develop branch from eba5e19 to e5f42fa Compare June 3, 2024 22:15

Merge branch 'develop' into 22697modularity

03a0823

	return len(_modular_polynomial(group, p, j).roots()) == 0
	return not _modular_polynomial(group, p, j).roots()

	r"""Helper function for the method :meth:`is_modular`.
	r"""
	Helper function for the method :meth:`is_modular`.


		INPUT:

		- ``verbose`` (bool, default ``False``) -- if True, outputs a reason for the conclusion.

		print("Modularity not established: this curve has small image at 3 and 5")

		return False # We've run out of tricks!

		return (j-1728).is_square()

		raise NotImplementedError("Only implemented for p==2")

	a model which is miniaml at all primes except one. Another difference
	a model which is minimal at all primes except one. Another difference

	sage: EllipticCurve('15a1').change_ring(K).rank()
	sage: EllipticCurve('15a1').change_ring(K).rank() # long time

	raise ValueError("p = {} should be a prime integer".format(p))
	raise ValueError(f"{p = } should be a prime integer")

Implement modularity test for elliptic curves #34989

Are you sure you want to change the base?

Implement modularity test for elliptic curves #34989

Conversation

JohnCremona commented Feb 6, 2023 • edited Loading

yyyyx4 commented Feb 6, 2023 • edited Loading

JohnCremona commented Feb 6, 2023

codecov-commenter commented Feb 8, 2023 • edited Loading

Codecov Report

JohnCremona commented Feb 8, 2023

JohnCremona commented Feb 8, 2023

tscrim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tscrim commented Feb 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohnCremona commented Feb 19, 2023

JohnCremona commented Feb 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tscrim commented Feb 24, 2023

JohnCremona commented Feb 24, 2023

tornaria left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohnCremona commented Feb 27, 2023

JohnCremona commented Feb 28, 2023

github-actions bot commented Nov 15, 2023 • edited Loading

fchapoton commented Sep 26, 2024

JohnCremona commented Sep 26, 2024

JohnCremona commented Jan 31, 2025

JohnCremona commented Feb 6, 2023 •

edited

Loading

yyyyx4 commented Feb 6, 2023 •

edited

Loading

codecov-commenter commented Feb 8, 2023 •

edited

Loading

github-actions bot commented Nov 15, 2023 •

edited

Loading