sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.38k stars 470 forks source link

doctest failure in schemes/elliptic_curves/heegner.py #24075

Closed videlec closed 6 years ago

videlec commented 6 years ago

On sage 8.1.beta8 several people got

sage -t --long src/sage/schemes/elliptic_curves/heegner.py
**********************************************************************
File "src/sage/schemes/elliptic_curves/heegner.py", line 6622, in
sage.schemes.elliptic_curves.heegner.heegner_index
Failed example:
     E.heegner_index(-8)
Expected:
     Traceback (most recent call last):
     ...
     RuntimeError: ...
Got:
     1.00000?
**********************************************************************

See original report on the sage-release thread.

Component: elliptic curves

Author: Frédéric Chapoton

Branch: 261e255

Reviewer: John Cremona

Issue created by migration from https://trac.sagemath.org/ticket/24075

videlec commented 6 years ago

Description changed:

--- 
+++ 
@@ -16,4 +16,4 @@
 **********************************************************************

-See original report on [https://groups.google.com/forum/#!topic/sage-release/r920G2EVTBU the sage-release thread](https://groups.google.com/forum/#!topic/sage-release/r920G2EVTBU the sage-release thread). +See original report on the sage-release thread.

JohnCremona commented 6 years ago
comment:2

This test uses the elliptic curve with Cremona label 274627a1 which is in the optional database, and has rank 1. Anyone running the test without the database will be computing the rank and that cannot be done using defaults (i.e. just doing E.rank()) as the generator is quite large. Those people seeing this fault probably have the database installed, so that the rank is set to 1 when te curve is created.

We can switch the test to a curve of conductor >400000, or manually set its rank to 1 using E._set_rank(). Or we can make this test optional on have the large database installed.

jdemeyer commented 6 years ago
comment:4

Nice analysis. So this is caused by #23962.

I would like to use a different curve which is not in any database.

JohnCremona commented 6 years ago
comment:5

Replying to @jdemeyer:

Nice analysis. So this is caused by #23962.

I would like to use a different curve which is not in any database.

I'll try to come up with one -- it should have rank 1 but E.rank() should fail to give that.

JohnCremona commented 6 years ago
comment:6

Try this:

sage: E = EllipticCurve([1,-1,0,-1228,-16267])
sage: E.conductor()
99982889
sage: E.heegner_index(-8)
Unable to compute the rank with certainty (lower bound=0).
This could be because Sha(E/Q)[2] is nontrivial.
Try calling something like two_descent(second_limit=13) on the
curve then trying this command again.  You could also try rank
with only_use_mwrank=False.
---------------------------------------------------------------------------
RuntimeError                              Traceback (most recent call last)
<ipython-input-3-367e86799b13> in <module>()
----> 1 E.heegner_index(-Integer(8))

/usr/local/sage/sage-2/local/lib/python2.7/site-packages/sage/schemes/elliptic_curves/heegner.pyc in heegner_index(self, D, min_p, prec, descent_second_limit, verbose_mwrank, check_rank)
   6522         raise ArithmeticError("Discriminant (=%s) must be a fundamental discriminant that satisfies the Heegner hypothesis."%D)
   6523 
-> 6524     if check_rank and self.rank() >= 2:
   6525         return rings.infinity
   6526 

/usr/local/sage/sage-2/local/lib/python2.7/site-packages/sage/schemes/elliptic_curves/ell_rational_field.pyc in rank(self, use_database, verbose, only_use_mwrank, algorithm, proof)
   2120                     print("with only_use_mwrank=False.")
   2121                     del E.__mwrank_curve
-> 2122                     raise RuntimeError('Rank not provably correct.')
   2123                 else:
   2124                     misc.verbose("Warning -- rank not proven correct", level=1)

RuntimeError: Rank not provably correct.
sage: E.heegner_index(-8, descent_second_limit=16, check_rank=False)
2.00000?

I only put the conductor line in to show that this is beyond the database. (Sorry, it is in Stein-Watkins.) Note that the correct Heegner index is 2 not 1 as in the other example.

I'll look for an example outside SW if you want but not right now.

JohnCremona commented 6 years ago
comment:7

BTW even if the huge SW database is installed this function would not be affected, so this example should be good. Finding examples is not that easy: E should have rank 1 and the quadratic twist used rank 0. At least one of those ranks should fail to be computed by default but succeed with the additional parameters given.

If people are happy with the new example I already posted, it would be great if someone else would make the patch; I'll review it. It need not be a blocker though, surely?

videlec commented 6 years ago
comment:8

It definitely is a blocker: an optional package is breaking a doctest.

fchapoton commented 6 years ago

Author: Frédéric Chapoton

fchapoton commented 6 years ago
comment:9

branch done


New commits:

261e255trac 24075 another example in heegner
fchapoton commented 6 years ago

Commit: 261e255

fchapoton commented 6 years ago

Branch: u/chapoton/24075

JohnCremona commented 6 years ago

Reviewer: John Cremona

JohnCremona commented 6 years ago
comment:10

This worked for me on a system with the optional database attached. I assume that the patchbots are testing with that -- I cannot interpret the variety of results various patchbots get but since the issue was to have an example which behaves the right way even with the optional database, this looks good.

vbraun commented 6 years ago

Changed branch from u/chapoton/24075 to 261e255

koffie commented 6 years ago
comment:13

Hi John,

Just in case you want to have some help interpreting failing patchbots sometime in the future: on #23832 there is a list of known failures for patchbots, there is also a link to it called "metaticket for patchbot failures" on trac.sagemath.org .The reason we considered this ticket a blocker is because it was also causing patchbots to fail and thereby hindering the sage development process.

koffie commented 6 years ago

Changed commit from 261e255 to none

videlec commented 6 years ago
comment:14

Note also that some patchbots are run with optional packages and some are not... the information is somewhere hidden in the log

Using --optional=ccache,gdb,mpir,python2,sage

versus

Using --optional=4ti2,autotools,benzene,bliss,boost,buckygen,cmake,coxeter3,cryptominisat,csdp,d3js,database_jones_numfield,database_kohel,database_mutation_class,database_odlyzko_zeta,database_pari,database_stein_watkins,database_stein_watkins_mini,database_symbolic_data,deformation,dot2tex,frobby,gambit,gap_jupyter,gp2c,igraph,latte_int,libbraiding,libhomfly,libogg,lidia,lrslib,mcqd,meataxe,mpfrcx,mpir,nose,notedown,openssl,ore_algebra,pandoc_attributes,pandocfilters,pari_jupyter,plantri,pysingular,python2,python_igraph,pyx,qhull,saclib,sage,scons,singular_jupyter,sirocco,tdlib,termcap,tides,topcom
JohnCremona commented 6 years ago
comment:15

OK, thanks both.