Opened 11 years ago

Closed 10 years ago

## #12341 closed defect (fixed)

# Empty (full) cremona database in Sage 4.8 causes tests to fail

Reported by: | Christopher Swenson | Owned by: | John Cremona |
---|---|---|---|

Priority: | critical | Milestone: | sage-5.5 |

Component: | elliptic curves | Keywords: | |

Cc: | Merged in: | sage-5.5.beta1 | |

Authors: | R. Andrew Ohana | Reviewers: | John Cremona |

Report Upstream: | N/A | Work issues: | |

Branch: | Commit: | ||

Dependencies: | Stopgaps: |

### Description (last modified by )

I am on Ubuntu 11.10, and noticed that a fresh install of Sage 4.8 fails a bunch of tests because the full cremona database is an empty file by default.

[swenson@harry 10:18:14] sage-4.8 :) $ ./sage -t /home/swenson/sage-src/sage-4.8/devel/sage-main/sage/misc/functional.py (hg)-[default]- sage -t "devel/sage-main/sage/misc/functional.py" #0: simplify_sum(expr='sum(q^k,k,0,inf)) #1: simplify_sum(expr=a*'sum(q^k,k,0,inf)) ********************************************************************** File "/home/swenson/sage-src/sage-4.8/devel/sage-main/sage/misc/functional.py", line 1360: sage: regulator(EllipticCurve('11a')) Exception raised: Traceback (most recent call last): File "/home/swenson/sage-src/sage-4.8/local/bin/ncadoctest.py", line 1231, in run_one_test self.run_one_example(test, example, filename, compileflags) File "/home/swenson/sage-src/sage-4.8/local/bin/sagedoctest.py", line 38, in run_one_example OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags) File "/home/swenson/sage-src/sage-4.8/local/bin/ncadoctest.py", line 1172, in run_one_example compileflags, 1) in test.globs File "<doctest __main__.example_54[3]>", line 1, in <module> regulator(EllipticCurve('11a'))###line 1360: sage: regulator(EllipticCurve('11a')) File "/home/swenson/sage-src/sage-4.8/local/lib/python/site-packages/sage/schemes/elliptic_curves/constructor.py", line 295, in EllipticCurve return ell_rational_field.EllipticCurve_rational_field(x) File "/home/swenson/sage-src/sage-4.8/local/lib/python/site-packages/sage/schemes/elliptic_curves/ell_rational_field.py", line 198, in __init__ X = sage.databases.cremona.CremonaDatabase()[label] File "/home/swenson/sage-src/sage-4.8/local/lib/python/site-packages/sage/databases/cremona.py", line 1363, in CremonaDatabase _db = LargeCremonaDatabase(name) File "/home/swenson/sage-src/sage-4.8/local/lib/python/site-packages/sage/databases/cremona.py", line 1119, in __init__ + 'not appear to be a valid SQL Cremona database.') RuntimeError: Database at /home/swenson/sage-src/sage-4.8/data//cremona/cremona.db does not appear to be a valid SQL Cremona database. ********************************************************************** 1 items had failures: 1 of 5 in __main__.example_54 ***Test Failed*** 1 failures. For whitespace errors, see the file /home/swenson/.sage//tmp/functional_29532.py [4.3 s] ---------------------------------------------------------------------- The following tests failed: sage -t "devel/sage-main/sage/misc/functional.py" Total time for all tests: 4.3 seconds [swenson@harry 10:18:22] sage-4.8 :( $ rm /home/swenson/sage-src/sage-4.8/data/cremona/cremona.db (hg)-[default]- [swenson@harry 10:18:25] sage-4.8 :) $ ./sage -t /home/swenson/sage-src/sage-4.8/devel/sage-main/sage/misc/functional.py (hg)-[default]- sage -t "devel/sage-main/sage/misc/functional.py" [4.3 s] ---------------------------------------------------------------------- All tests passed! Total time for all tests: 4.3 seconds

See comment:14 for an explanation of the issues causing this.

Apply attachment:trac_12341.patch to the Sage Library.

### Attachments (1)

### Change History (31)

### comment:1 Changed 11 years ago by

### comment:2 Changed 11 years ago by

Priority: | major → critical |
---|

I notice the spkg-install inside elliptic_curves-0.3/cremona_mini has code like

cremona_root=os.environ['SAGE_DATA']+'/cremona' if not os.path.exists(cremona_root): os.makedirs(cremona_root) target=cremona_root+'/cremona_mini.db'

This is bad form; one should use os.path.join.

### comment:3 Changed 11 years ago by

The following should be using xreadlines, to save memory:

for line in file('src/allcurves.00000-09999').readlines():

This optional test in cremona.py is bad, since it will always fail when the optional package is installed.

sage: c = CremonaDatabase() sage: isinstance(c, sage.databases.cremona.MiniCremonaDatabase) True sage: isinstance(c, sage.databases.cremona.LargeCremonaDatabase) # optional - database_cremona_ellcurve True

Also, the name of the optional component is totally wrong -- it should be elliptic_curves, which is the name of the spkg:

sage-4.8/spkg/standard/elliptic_curves-0.3.spkg

Anyway, trac #11587 unfortunately introduced many issues. Let's fix them asap for 5.0!

### comment:5 follow-up: 7 Changed 11 years ago by

This appeared to be caused by, at some point, I must have run

./sage -t --optional devel/sage/sage/databases/cremona.py

This will create an empty cremona database as a side effect, even though it should not.

### comment:7 Changed 11 years ago by

Replying to swenson:

This appeared to be caused by, at some point, I must have run

`./sage -t --optional devel/sage/sage/databases/cremona.py`

This will create an empty cremona database as a side effect, even though it should not.

I think the culprit is line 1088:

sage: c = CremonaDatabase('cremona') # optional

The issue is because I didn't check for `read_only=False`

before opening a non-existent database. Shouldn't be too hard to fix.

-- Andrew

### comment:8 Changed 11 years ago by

Status: | new → needs_review |
---|

Hopefully that fixes this issue. Still a lot of things that could be improved as William noted.

### comment:9 follow-up: 10 Changed 11 years ago by

As one of the reviewers for #11587, I apologise. I'm not sure what I should have done differently, other than being clever when reading the new code there. This was rather a complicated change to make since both before and after there were standard and optional spkgs involved. I cannot have tested all possible combinations. At least now we only have two combinations to check (before and after installing the optional large database). All the 4.8 builds I have already have the large database installed though, so unless someone tells me how to uninstall it I cannot test the effect of this patch!

### comment:10 Changed 11 years ago by

I realized that my patch only half fixes the problem, the creation of and empty full database. The other half is for it to not use that file unless it sees that the full database is installed. I most likely won't be able to get around to this today.

Replying to cremona:

As one of the reviewers for #11587, I apologise. This was rather a complicated change to make since both before and after there were standard and optional spkgs involved.

Yeah, I'm not really surprised by bugs popping up, it was a major change.

### comment:11 Changed 11 years ago by

Status: | needs_review → needs_work |
---|

### comment:12 Changed 11 years ago by

Authors: | → R. Andrew Ohana |
---|---|

Status: | needs_work → needs_review |

Ok, I've updated the patch. The optional spkg is here if you would like to test with it (the spkg repo hasn't been updated yet).

### comment:13 follow-up: 14 Changed 11 years ago by

Reviewers: | → John Cremona |
---|

Patch applies fine and all elliptic_curves tests pass both before and after installing the optional database (which with 5.0.bets5 I installed with a plain "sage -i database_cremona_ellcurve").

I have not checked all the logic in the revised code. I was puzzled by the remark about testing with flag --optional since I thought that only made sense with some sensible string after the word "optional", e.g. "magma". But it seems that there are doctest lines which are tagged "# optional" with no mention of which optional package is relevant. Possibly for this reason, I had weird failures with

sage -t --optional devel/sage-main/sage/schemes/elliptic_curves/lseries_ell.py # 3 doctests failed

which may have nothing to do with this ticket.

I hesitate to set this to Positive Review since I really don't understand what it going on, but at least I have done something and reported what has happened.

### comment:14 Changed 11 years ago by

Description: | modified (diff) |
---|

Replying to cremona:

I have not checked all the logic in the revised code.

I don't think I have explained the issue that this ticket fixes very clearly (which was exposed when running one of the optional doctests without having the optional spkg installed). There are 2 main issues that caused this:

- When calling
`CremonaDatabase('name')`

the file`SAGE_ROOT/data/cremona/name.db`

was being touched, something that is bad in general if the file wasn't there to start with.

- When calling
`CremonaDatabase()`

, to determine if the full database is installed, it would check for the existence of`SAGE_ROOT/data/cremona/cremona.db`

rather than use one of the functions in`sage.misc`

for this purpose.

With these combined, running the optional doctests would break the cremona database: it would run `CremonaDatabase('cremona')`

, which would touch `SAGE_ROOT/data/cremona/cremona.db`

and thus later calls of `CremonaDatabase()`

would break since it would think the full cremona database was installed and try to use the touched file.

I was puzzled by the remark about testing with flag --optional since I thought that only made sense with some sensible string after the word "optional", e.g. "magma".

Unless you want to test all optional packages (which could be useful for a giant system install -- unfortunately there are a few optional packages that don't want to build properly at the moment).

But it seems that there are doctest lines which are tagged "# optional" with no mention of which optional package is relevant.

I consider this a bug and something that should be required for any optional tags.

### comment:15 Changed 10 years ago by

Description: | modified (diff) |
---|

The second patch (which includes the changes in the previous one) addresses the cases where there is a tag #optional in the code but no indication of which optional pakage is required, for the elliptic_curves directory only.

With sage-5.4.beta1 with no optional spkgs installed, all tests pass if -optional is not given, while if -optional is given then there are two failures in elliptic_curve/, both due to {{{database_cremona_ellcurve}} not being installed. After installing that one optional spkg, all these pass. That is as it should be.

Since I added to the patch it needs a second referee.

### comment:16 Changed 10 years ago by

ok, patch has been rebased against recent changes (removing `SAGE_DATA`

in favor of `SAGE_LOCAL/share`

)

### comment:18 Changed 10 years ago by

I can confirm that trac_12341.patch applies cleanly to 5.4.rc1 and all tests in sage/schemes/elliptic_curves still pass.

Andrew, if you are happy with the (very small) changes I had made to your first patch, between us we can make this a positive review.

Note that #12565 depends on this...

### comment:19 Changed 10 years ago by

I'm happy with the small changes you made, shall we mark this as positive review?

### comment:20 Changed 10 years ago by

Status: | needs_review → positive_review |
---|

### comment:21 follow-up: 24 Changed 10 years ago by

Status: | positive_review → needs_work |
---|

#optional - database_cremona_ellcurve (conductor is greater than 10000)

should be

# optional - database_cremona_ellcurve

(you're not supposed to put extra stuff after "optional")

### comment:22 Changed 10 years ago by

Milestone: | sage-5.4 → sage-5.5 |
---|

### comment:23 Changed 10 years ago by

Description: | modified (diff) |
---|

### comment:24 Changed 10 years ago by

Replying to jdemeyer:

#optional - database_cremona_ellcurve (conductor is greater than 10000)should be

# optional - database_cremona_ellcurve(you're not supposed to put extra stuff after "optional")

Fixed -- please may I have my positive review back now?

### comment:25 Changed 10 years ago by

Status: | needs_work → positive_review |
---|

### comment:26 Changed 10 years ago by

Status: | positive_review → needs_work |
---|

I'm afraid this needs to be rebased to sage-5.4.rc1:

applying trac_12341a.patch patching file sage/databases/cremona.py Hunk #1 FAILED at 51 Hunk #2 FAILED at 113 Hunk #4 FAILED at 593 Hunk #7 FAILED at 1317 Hunk #9 FAILED at 1567 5 out of 9 hunks FAILED -- saving rejects to file sage/databases/cremona.py.rej patch failed, unable to continue (try -v) patch failed, rejects left in working dir errors during apply, please fix and refresh trac_12341a.patch

### comment:27 follow-up: 28 Changed 10 years ago by

Description: | modified (diff) |
---|---|

Status: | needs_work → needs_review |

ok, re-rebased against 5.4.rc1 (it seems that my rebased changes disappeared when John went to fix the optional doctest flag)

### comment:28 Changed 10 years ago by

Replying to ohanar:

ok, re-rebased against 5.4.rc1 (it seems that my rebased changes disappeared when John went to fix the optional doctest flag)

Very sorry, that was entirely my fault as your rebased patch had the same name as the previous one; I had the old patch on my machine and just edited the patch file without downloading the new version.

### comment:29 Changed 10 years ago by

Status: | needs_review → positive_review |
---|

There is one small remaining issue here, which admittedly is not caused by theis ticket/patch. Namely, all tests pass with this patch without the optional database installed, but when it is installed there is a doctest error on line 6454 of elliptic_curves/heegner.py. That needs to be fixed somewhere, so that it passes whether or not the optional db is installed. But I am not sure how to do that: it is an example to illustrate a specific behaviour, so the usual trick of using a curve whose conductor is so large it will not be in the optional database in the forseeable future will not do.

It seems bad to hold up this ticket for this reason! So I am giving this a positive review, and refer to #13547 to address the remaining issue.

### comment:30 Changed 10 years ago by

Merged in: | → sage-5.5.beta1 |
---|---|

Resolution: | → fixed |

Status: | positive_review → closed |

**Note:**See TracTickets for help on using tickets.

Also confirmed on my OS X 10.6 with a stock Sage 4.8 install.