Opened 5 years ago
Closed 5 years ago
#23840 closed defect (fixed)
Doctest failures in ell_number_field.py
Reported by:  Jeroen Demeyer  Owned by:  

Priority:  blocker  Milestone:  sage8.1 
Component:  doctest coverage  Keywords:  
Cc:  Vincent Delecroix  Merged in:  
Authors:  John Cremona  Reviewers:  Jeroen Demeyer 
Report Upstream:  N/A  Work issues:  
Branch:  453419e (Commits, GitHub, GitLab)  Commit:  453419e242f8f3e9dcafa6713a13151144df28e2 
Dependencies:  Stopgaps: 
Description (last modified by )
When the optional package database_cremona_ellcurve is installed, doctest failures happen (see below). The underlying reason is that the Cremona database contains a different generator for elliptic curve 37a1
than the one computed with the .gens()
method (the generators are each other's negative):
sage: EllipticCurve("37a1")._compute_gens(proof=True, use_database=True) ([(0 : 0 : 1)], True) sage: EllipticCurve("37a1")._compute_gens(proof=True, use_database=False) ([(0 : 1 : 1)], True)
Doctest failures:
sage t long warnlong 74.4 src/sage/schemes/elliptic_curves/ell_number_field.py ********************************************************************** File "src/sage/schemes/elliptic_curves/ell_number_field.py", line 2457, in sage.schemes.elliptic_curves.ell_number_field.EllipticCurve_number_field.reduction.gens Failed example: E.gens() Expected: [(2 : 1/2*a  1/2 : 1), (0 : 1 : 1)] Got: [(2 : 1/2*a  1/2 : 1), (0 : 0 : 1)] ********************************************************************** File "src/sage/schemes/elliptic_curves/ell_number_field.py", line 2841, in sage.schemes.elliptic_curves.ell_number_field.EllipticCurve_number_field.reduction.isogeny_class Warning, slow doctest: CL = EL.isogeny_class(); len(CL) # long time (~80s) Test ran for 118.41 s ********************************************************************** File "src/sage/schemes/elliptic_curves/ell_number_field.py", line 3310, in sage.schemes.elliptic_curves.ell_number_field.EllipticCurve_number_field.reduction.lll_reduce Failed example: E.lll_reduce(E.gens()) Expected: ( [0 1] [(0 : 1 : 1), (2 : 1/2*a  1/2 : 1)], [1 0] ) Got: ( [0 1] [(0 : 0 : 1), (2 : 1/2*a  1/2 : 1)], [1 0] ) ********************************************************************** File "src/sage/schemes/elliptic_curves/ell_number_field.py", line 3607, in sage.schemes.elliptic_curves.ell_number_field.EllipticCurve_number_field.reduction.? Failed example: EK.saturation([2*P, Q], max_prime=2) Expected: ([(1 : 1 : 1), (0 : 1 : 1)], 2, 0.152460177943144) Got: ([(1 : 1 : 1), (0 : 0 : 1)], 2, 0.152460177943144) ********************************************************************** File "src/sage/schemes/elliptic_curves/ell_number_field.py", line 3609, in sage.schemes.elliptic_curves.ell_number_field.EllipticCurve_number_field.reduction.? Failed example: EK.saturation([P+Q, PQ], lower_ht_bound=.1, debug=2) Expected: ([(1 : 1 : 1), (1 : 0 : 1)], 2, 0.152460177943144) Got: ([(1 : 1 : 1), (4 : 8 : 1)], 2, 0.152460177943144) ********************************************************************** File "src/sage/schemes/elliptic_curves/ell_number_field.py", line 3611, in sage.schemes.elliptic_curves.ell_number_field.EllipticCurve_number_field.reduction.? Failed example: EK.saturation([P+Q, 17*Q], lower_ht_bound=0.1) Expected: ([(4 : 8 : 1), (0 : 1 : 1)], 17, 0.152460177943143) Got: ([(1 : 0 : 1), (0 : 0 : 1)], 17, 0.152460177943144) ********************************************************************** File "src/sage/schemes/elliptic_curves/ell_number_field.py", line 3615, in sage.schemes.elliptic_curves.ell_number_field.EllipticCurve_number_field.reduction.? Failed example: P, Q, R Expected: ((i  2 : i  3 : 1), (1 : 1 : 1), (0 : 1 : 1)) Got: ((i + 1 : 2*i  1 : 1), (1 : 1 : 1), (0 : 0 : 1)) ********************************************************************** File "src/sage/schemes/elliptic_curves/ell_number_field.py", line 3617, in sage.schemes.elliptic_curves.ell_number_field.EllipticCurve_number_field.reduction.? Failed example: EK.saturation([P+Q, Q+R, R+P], lower_ht_bound=0.1) Expected: ([(841/1369*i  171/1369 : 41334/50653*i  74525/50653 : 1), (4 : 8 : 1), (1/25*i + 18/25 : 69/125*i  58/125 : 1)], 2, 0.103174443217351) Got: ([(5*i : 9*i  8 : 1), (1 : 0 : 1), (1/2*i : 3/4*i  5/4 : 1)], 2, 0.103174443217351) ********************************************************************** File "src/sage/schemes/elliptic_curves/ell_number_field.py", line 3623, in sage.schemes.elliptic_curves.ell_number_field.EllipticCurve_number_field.reduction.? Failed example: EK.saturation([26*R], lower_ht_bound=0.1) Expected: ([(0 : 1 : 1)], 26, 0.327000773651605) Got: ([(0 : 0 : 1)], 26, 0.327000773651605) ********************************************************************** File "src/sage/schemes/elliptic_curves/ell_number_field.py", line 3632, in sage.schemes.elliptic_curves.ell_number_field.EllipticCurve_number_field.reduction.? Failed example: EK.saturation([P+Q, PQ], lower_ht_bound=0.1) Expected: ([(1 : 1 : 1), (1 : 0 : 1)], 2, 0.152460177943144) Got: ([(1 : 1 : 1), (4 : 8 : 1)], 2, 0.152460177943144) ********************************************************************** File "src/sage/schemes/elliptic_curves/ell_number_field.py", line 3634, in sage.schemes.elliptic_curves.ell_number_field.EllipticCurve_number_field.reduction.? Failed example: EK.saturation([3*P, P+5*Q], lower_ht_bound=0.1) Expected: ([(185/2209 : 119510/103823 : 1), (80041/34225 : 26714961/6331625 : 1)], 15, 0.152460177943144) Got: ([(14/25 : 216/125 : 1), (445/49 : 9955/343 : 1)], 15, 0.152460177943144) ********************************************************************** File "src/sage/schemes/elliptic_curves/ell_number_field.py", line 3645, in sage.schemes.elliptic_curves.ell_number_field.EllipticCurve_number_field.reduction.? Failed example: EK.saturation([3*PQ, 3*P+Q], lower_ht_bound=.01) Expected: ([(a + 2 : 2*a  4 : 1), (8820833592677/3284478409969*a + 13569900067225/3284478409969 : 43082636045850669307/5952502920606148297*a + 76367500748298491757/5952502920606148297 : 1)], 6, 0.0317814530725985) Got: ([(a + 2 : 2*a + 3 : 1), (8820833592677/3284478409969*a + 13569900067225/3284478409969 : 43082636045850669307/5952502920606148297*a  82320003668904640054/5952502920606148297 : 1)], 6, 0.0317814530725985) ********************************************************************** File "src/sage/schemes/elliptic_curves/ell_number_field.py", line 3679, in sage.schemes.elliptic_curves.ell_number_field.EllipticCurve_number_field.reduction.? Failed example: EK.saturation([P+Q, PQ], lower_ht_bound=.1, debug=2) Expected: ([(1 : 1 : 1), (1 : 0 : 1)], 2, 0.152460177943144) Got: ([(1 : 1 : 1), (4 : 8 : 1)], 2, 0.152460177943144) ********************************************************************** File "src/sage/schemes/elliptic_curves/ell_number_field.py", line 3681, in sage.schemes.elliptic_curves.ell_number_field.EllipticCurve_number_field.reduction.? Failed example: EK.saturation([5*P+6*Q, 5*P3*Q], lower_ht_bound=.1) Expected: ([(3/4 : 15/8 : 1), (159965/16129 : 67536260/2048383 : 1)], 45, 0.152460177943144) Got: ([(51/25 : 68/125 : 1), (1433882253389/379703672401 : 1707565715369408803/233973782637168601 : 1)], 45, 0.152460177943144) ********************************************************************** File "src/sage/schemes/elliptic_curves/ell_number_field.py", line 3685, in sage.schemes.elliptic_curves.ell_number_field.EllipticCurve_number_field.reduction.? Failed example: EK.saturation([5*P+6*Q, 5*P3*Q], lower_ht_bound=.1, debug=2) Expected: ([(3/4 : 15/8 : 1), (159965/16129 : 67536260/2048383 : 1)], 45, 0.152460177943144) Got: ([(51/25 : 68/125 : 1), (1433882253389/379703672401 : 1707565715369408803/233973782637168601 : 1)], 45, 0.152460177943144) ********************************************************************** File "src/sage/schemes/elliptic_curves/ell_number_field.py", line 3766, in sage.schemes.elliptic_curves.ell_number_field.EllipticCurve_number_field.reduction.gens_quadratic Failed example: E.change_ring(QuadraticField(1, 'i')).gens_quadratic() Expected: [(0 : 1 : 1)] Got: [(0 : 0 : 1)] ********************************************************************** File "src/sage/schemes/elliptic_curves/ell_number_field.py", line 3768, in sage.schemes.elliptic_curves.ell_number_field.EllipticCurve_number_field.reduction.gens_quadratic Failed example: E.change_ring(QuadraticField(3, 'i')).gens_quadratic() Expected: [(i + 2 : 2*i  4 : 1), (1/12 : 17/72*i  1/2 : 1)] Got: [(i + 2 : 2*i + 3 : 1), (1/12 : 17/72*i  1/2 : 1)] ********************************************************************** File "src/sage/schemes/elliptic_curves/ell_number_field.py", line 3772, in sage.schemes.elliptic_curves.ell_number_field.EllipticCurve_number_field.reduction.gens_quadratic Failed example: EllipticCurve('11a').change_ring(K).gens_quadratic() Expected: [(6 : 11/2*a  1/2 : 1)] Got: [(6 : 11/2*a  1/2 : 1)] ********************************************************************** File "src/sage/schemes/elliptic_curves/ell_number_field.py", line 3774, in sage.schemes.elliptic_curves.ell_number_field.EllipticCurve_number_field.reduction.gens_quadratic Failed example: EllipticCurve('389a').change_ring(K).gens_quadratic() Expected: [(1/8*a + 5/8 : 5/16*a  9/16 : 1), (1 : 1 : 1), (0 : 1 : 1)] Got: [(1/2*a  1/2 : 1/2*a  5/2 : 1), (1 : 1 : 1), (0 : 0 : 1)] ********************************************************************** 4 items had failures: 12 of 35 in sage.schemes.elliptic_curves.ell_number_field.EllipticCurve_number_field.reduction.? 1 of 18 in sage.schemes.elliptic_curves.ell_number_field.EllipticCurve_number_field.reduction.gens 4 of 9 in sage.schemes.elliptic_curves.ell_number_field.EllipticCurve_number_field.reduction.gens_quadratic 1 of 25 in sage.schemes.elliptic_curves.ell_number_field.EllipticCurve_number_field.reduction.lll_reduce [766 tests, 18 failures, 273.84 s]  sage t long warnlong 74.4 src/sage/schemes/elliptic_curves/ell_number_field.py # 18 doctests failed 
Change History (30)
comment:1 Changed 5 years ago by
Description:  modified (diff) 

Summary:  Doctest failures in ell_number_field.py with optional packages → Doctest failures in ell_number_field.py 
comment:2 Changed 5 years ago by
Description:  modified (diff) 

comment:3 Changed 5 years ago by
Description:  modified (diff) 

comment:4 followup: 5 Changed 5 years ago by
comment:5 Changed 5 years ago by
Replying to cremona:
These can all just be changed to match the output. There is nothing more serious here than using +P instead of P as a generator of an infinite cyclic group, and similarly for rank 2. I went through all these making similar changes when testing #8829, and now they seem to have changed again (or gone back).
There is no way to have an output dependent on an optional package... the following does not exist
sage: my_test() +1 # in sage 1 # optional  cremona
For testing only, we could obviously have tests which did not reply on a specific set of Zmodule generators being output. But we want to show examples for the documentation too, and these really do want actual points output. It's a universal problem.
comment:6 followup: 7 Changed 5 years ago by
I don't understand your comment Vincent! I was not suggesting such a thing, only that the output of E.gens() and similar often change when some underlying package (such as pari) changes.
comment:7 Changed 5 years ago by
Replying to cremona:
I don't understand your comment Vincent! I was not suggesting such a thing, only that the output of E.gens() and similar often change when some underlying package (such as pari) changes.
Then I don't understand your comment:4 either ;) What do you suggest with the following sentence
These can all just be changed to match the output.
I don't believe that E.gens()
is uniformly minus the generators of what we have with the cremona database installed.
comment:8 Changed 5 years ago by
The problem is that here the output depends on whether or not the optional package database_cremona_ellcurve
is installed. So with that package, you get one output and without that package, you get a different output. The difference boils down to
sage: EllipticCurve("37a1")._compute_gens(proof=True, use_database=True) ([(0 : 0 : 1)], True) sage: EllipticCurve("37a1")._compute_gens(proof=True, use_database=False) ([(0 : 1 : 1)], True)
comment:9 Changed 5 years ago by
Priority:  critical → blocker 

comment:10 Changed 5 years ago by
OK, this is clearer now. As the ticket is about elliptic curves over number fields I did not think it would be affected by the optional database, but in fact it is since some of these examples take an elliptic curve over Q and basechange them up. What we did for such examples over Q is not to use the gens() method at all but just hardcode the points in the examples. I volunteer to do that  but all the computers I use have the optional database installed (well, what would you expect?!) so it is harder for me to test. I will not do this today though.
comment:11 Changed 5 years ago by
Even if you do not do this today, do you have a suggestion for fixing it?
comment:12 Changed 5 years ago by
Yes my suggestion was in Comment 10, i.e. don't get gens from the database.
comment:13 followup: 14 Changed 5 years ago by
Taking a step back: Sage generates a "mini" Cremona database as standard package, containing only curves of conductor up to 10000. That mini database does not contain the generators of the curves. Should it?
comment:14 followup: 15 Changed 5 years ago by
Replying to jdemeyer:
Taking a step back: Sage generates a "mini" Cremona database as standard package, containing only curves of conductor up to 10000. That mini database does not contain the generators of the curves. Should it?
Probably, if only for this reason concerning tests, but for anyone who cares at all about elliptic curves installing the optional package is obviously a good idea.
comment:15 followups: 16 20 Changed 5 years ago by
Replying to cremona:
Replying to jdemeyer:
Taking a step back: Sage generates a "mini" Cremona database as standard package, containing only curves of conductor up to 10000. That mini database does not contain the generators of the curves. Should it?
Probably, if only for this reason concerning tests, but for anyone who cares at all about elliptic curves installing the optional package is obviously a good idea.
The mini database does not have the same columns as the big one!?
comment:16 Changed 5 years ago by
Replying to vdelecroix:
Replying to cremona:
Replying to jdemeyer:
Taking a step back: Sage generates a "mini" Cremona database as standard package, containing only curves of conductor up to 10000. That mini database does not contain the generators of the curves. Should it?
Probably, if only for this reason concerning tests, but for anyone who cares at all about elliptic curves installing the optional package is obviously a good idea.
The mini database does not have the same columns as the big one!?
That is correct. I didn't design it, it has been like that since 2007 if not earlier.
comment:17 followups: 18 21 Changed 5 years ago by
I am looking at this now. Note that there used to be a whole lot of discrepancies caused by the presence or otherwise of the optional database, which were all dealt with. The new batch come from #8829 and are my fault, I think, since I forgot. It is quite plausible that both I and the reviewer of that ticket had the optional database installed so it was not noticed until the patchbots picked it up.
There is no very easy way to uninstall the optional database but I'll try that after changing the relevant tests.
comment:18 Changed 5 years ago by
Replying to cremona:
I am looking at this now. Note that there used to be a whole lot of discrepancies caused by the presence or otherwise of the optional database, which were all dealt with. The new batch come from #8829 and are my fault, I think, since I forgot. It is quite plausible that both I and the reviewer of that ticket had the optional database installed so it was not noticed until the patchbots picked it up.
There is no very easy way to uninstall the optional database but I'll try that after changing the relevant tests.
And this is indeed a problem... however we might have #22510 in at some point! The only easy way I see is to have two installs of Sage.
comment:19 Changed 5 years ago by
This one should be easy since all the installation does is move one file into local/share/cremona. Checking that made me see that despite what I said above, I had just tested this file on a Sage build with no optional database installed  and all tests passed; but the version was beta3. I am now building beta5 in there before continuing.
comment:20 followup: 23 Changed 5 years ago by
Replying to vdelecroix:
The mini database does not have the same columns as the big one!?
Yes, that's the case. I have no idea why it is like this:
jdemeyer@tamiyo:/usr/local/src/sageconfig$ ./sage sqlite3 local/share/cremona/cremona.db SQLite version 3.17.0 20170213 16:02:40 Enter ".help" for usage hints. sqlite> .schema t_curve CREATE TABLE t_curve(reg REAL, om REAL, sha , tors INTEGER, eqn TEXT UNIQUE, cp INTEGER, curve TEXT PRIMARY KEY, class TEXT, gens TEXT); CREATE INDEX i_t_curve_class ON t_curve(class); sqlite> jdemeyer@tamiyo:/usr/local/src/sageconfig$ ./sage sqlite3 local/share/cremona/cremona_mini.db SQLite version 3.17.0 20170213 16:02:40 Enter ".help" for usage hints. sqlite> .schema t_curve CREATE TABLE t_curve(curve TEXT PRIMARY KEY, class TEXT, tors INTEGER, eqn TEXT UNIQUE); CREATE INDEX i_t_curve_class ON t_curve(class);
I vote for changing this. The mini database should be just a truncated version of the large database.
comment:21 followup: 22 Changed 5 years ago by
Replying to cremona:
It is quite plausible that both I and the reviewer of that ticket had the optional database installed so it was not noticed until the patchbots picked it up.
It is the other way around. Tests pass without the database, they fail with the database.
comment:22 Changed 5 years ago by
Replying to jdemeyer:
Replying to cremona:
It is quite plausible that both I and the reviewer of that ticket had the optional database installed so it was not noticed until the patchbots picked it up.
It is the other way around. Tests pass without the database, they fail with the database.
OK, I had just realised that. I now have beta5 without the database (and saw that tests pass) so am almost in a state where I can try to deal with this.
The first failure (line 2457) is an extremely weird example. With K a number field the construction EllipticCurve?(K,'37') really should not work. It appears to get curve '37a1' from the database and basechange it to K, but '37' does not uniquely determine the curve. This will change.
The trouble with that example (and probably more of the problem ones) is that we are testing something which was implemented whereby the generators of the basechange of a curve defined over Q from Q to a quadratic field are obtained by joining the gens of E and its quadratic twist and then saturating. So we are at the mercy of what generators two curves over Q get assigned. The only way I can think of avoiding the database issue is to use an example outside the database range. That is, until someone makes the suggested changes to the mini database; then we'll only need to make sure that any curves in tests like this are in the mini database.
comment:23 Changed 5 years ago by
Replying to jdemeyer:
I vote for changing this. The mini database should be just a truncated version of the large database.
Yes this looks like the cleanest solution to me to, I don't know how much work this would be, but if it is significantly more work we could postpone it to a later ticket.
comment:24 Changed 5 years ago by
Definitely changing the mini database, and the associated code, should not be in the *blocker* ticket!
Here is what to do: there exist functions already to take some data tarball to create the db file for the optional database. It would be easy to feed to that a smaller tarball with the range to 10000 only. Then in the code there might be some places where changes are needed, e.g. if the code assumes that the mini database does not have the gens and possibly some other fields.
comment:25 Changed 5 years ago by
I have a version of ell_number_field.py which should do the job as it no longer calls gens() on any elliptic curve over Q of conductor <400000. It passes all tests on 8.1.beta5 without the optiona db installed. I want to test on 8.0.beta5+database_cremona_ellcurve but am having trouble. I just built it in an upgrade from some earlier version and Sage runs but sage t gives
Traceback (most recent call last): File "/usr/local/sage/sage2/src/bin/sageruntests", line 111, in <module> from sage.doctest.control import DocTestController File "/usr/local/sage/sage2/local/lib/python2.7/sitepackages/sage/doctest/control.py", line 33, in <module> from .sources import FileDocTestSource, DictAsObject File "/usr/local/sage/sage2/local/lib/python2.7/sitepackages/sage/doctest/sources.py", line 33, in <module> from .parsing import SageDocTestParser File "/usr/local/sage/sage2/local/lib/python2.7/sitepackages/sage/doctest/parsing.py", line 56, in <module> from sage.all import RealIntervalField File "/usr/local/sage/sage2/local/lib/python2.7/sitepackages/sage/all.py", line 87, in <module> from sage.misc.all import * # takes a while File "/usr/local/sage/sage2/local/lib/python2.7/sitepackages/sage/misc/all.py", line 84, in <module> from .functional import (additive_order, File "/usr/local/sage/sage2/local/lib/python2.7/sitepackages/sage/misc/functional.py", line 27, in <module> from sage.rings.complex_double import CDF File "sage/rings/integer.pxd", line 7, in init sage.rings.complex_double File "sage/rings/rational.pxd", line 8, in init sage.rings.integer File "sage/rings/rational.pyx", line 89, in init sage.rings.rational File "sage/rings/real_double.pxd", line 8, in init sage.rings.real_mpfr ImportError: /usr/local/sage/sage2/local/lib/python2.7/sitepackages/sage/rings/real_double.so: failed to map segment from shared object
which makes no sense to me. I can make clean and start again but that will take a while. I have another beta5 build happening elsewhere. Meanwhile I'll upload the branch I have and someone else can try it.
comment:26 Changed 5 years ago by
Authors:  → John Cremona 

Branch:  → u/cremona/23840 
Commit:  → 453419e242f8f3e9dcafa6713a13151144df28e2 
Status:  new → needs_review 
New commits:
453419e  #23840: fix tests whose output depends on optional elliptic curve database being installed

comment:27 Changed 5 years ago by
I found out how to manually install the optional spkg in a way which allows for easy manual uninstallation:
 copy the file cremona.db into local/share/cremona
 copy the file local/var/lib/sage/installed/database_cremona_ellcurve20161017
in both cases copying from another build with this spkg installed.
The tests still pass  good! So I think this is now fixed.
comment:29 Changed 5 years ago by
Reviewers:  → Jeroen Demeyer 

Status:  needs_review → positive_review 
comment:30 Changed 5 years ago by
Branch:  u/cremona/23840 → 453419e242f8f3e9dcafa6713a13151144df28e2 

Resolution:  → fixed 
Status:  positive_review → closed 
These can all just be changed to match the output. There is nothing more serious here than using +P instead of P as a generator of an infinite cyclic group, and similarly for rank 2. I went through all these making similar changes when testing #8829, and now they seem to have changed again (or gone back).
For testing only, we could obviously have tests which did not reply on a specific set of Zmodule generators being output. But we want to show examples for the documentation too, and these really do want actual points output. It's a universal problem.