Opened 9 years ago
Closed 8 years ago
#10999 closed defect (fixed)
Elliptic curve generators from the database lie on the wrong curve
Reported by: | gagansekhon | Owned by: | cremona |
---|---|---|---|
Priority: | minor | Milestone: | sage-4.7.1 |
Component: | elliptic curves | Keywords: | Cremona database, integral points, gens |
Cc: | cremona, rlm, aly.deines, mstreng | Merged in: | sage-4.7.1.alpha4 |
Authors: | John Cremona | Reviewers: | Jamie Weigandt, Marco Streng |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
After installing the large cremona database. The following code shows something strange (which then causes a failure in E.integral_points()(:
sage: E=EllipticCurve('389a1') sage: [P.curve() is E for P in E.gens()] [False, False] sage: [P.curve() == E for P in E.gens()] [True, True]
There is no problem when the database is not installed, since then the generators are computed on E itself. But with the database, an isomorphism is applied to the generators on the database curve to this curve (in this case it is the identity map) and somewhere in that process E is replaced by a copy.
Attachments (1)
Change History (15)
comment:1 Changed 9 years ago by
- Description modified (diff)
- Summary changed from E.integral_points has a bug to Elliptic curve generators from the database lie on the wrong curve
comment:2 Changed 9 years ago by
comment:3 Changed 9 years ago by
I'm testing the whole library now and will ask for reviews if that goes ok.
comment:4 Changed 9 years ago by
- Status changed from new to needs_review
comment:5 follow-up: ↓ 6 Changed 9 years ago by
- Reviewers set to Jamie Weigandt
- Status changed from needs_review to positive_review
Looks good. The tests pass.
comment:6 in reply to: ↑ 5 Changed 9 years ago by
- Status changed from positive_review to needs_work
Replying to weigandt:
The tests pass.
Really? Both the patchbot and my personal testing seems to indicate doctest problems. Am I missing something?
********************************************************************** File "/mnt/usb1/scratch/jdemeyer/merger/sage-4.7.alpha3/devel/sage-main/doc/en/bordeaux_2008/elliptic_curves.rst", line 139: sage: E.padic_regulator(5, 10) Exception raised: Traceback (most recent call last): File "/mnt/usb1/scratch/jdemeyer/merger/sage-4.7.alpha3/local/bin/ncadoctest.py", line 1231, in run_one_test self.run_one_example(test, example, filename, compileflags) File "/mnt/usb1/scratch/jdemeyer/merger/sage-4.7.alpha3/local/bin/sagedoctest.py", line 38, in run_one_example OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags) File "/mnt/usb1/scratch/jdemeyer/merger/sage-4.7.alpha3/local/bin/ncadoctest.py", line 1172, in run_one_example compileflags, 1) in test.globs File "<doctest __main__.example_3[3]>", line 1, in <module> E.padic_regulator(Integer(5), Integer(10))###line 139: sage: E.padic_regulator(5, 10) File "/mnt/usb1/scratch/jdemeyer/merger/sage-4.7.alpha3/local/lib/python/site-packages/sage/schemes/elliptic_curves/padics.py", line 269, in padic_regulator d = self.padic_height_pairing_matrix(p=p, prec=prec, height=height, check_hypotheses=False) File "/mnt/usb1/scratch/jdemeyer/merger/sage-4.7.alpha3/local/lib/python/site-packages/sage/schemes/elliptic_curves/padics.py", line 344, in padic_height_pairing_matrix basis = self.gens() File "/mnt/usb1/scratch/jdemeyer/merger/sage-4.7.alpha3/local/lib/python/site-packages/sage/schemes/elliptic_curves/ell_rational_fie ld.py", line 1802, in gens return list(self.__gens[proof]) # return copy so not changed File "parent.pyx", line 738, in sage.structure.parent.Parent.__getattr__ (sage/structure/parent.c:5752) File "parent.pyx", line 177, in sage.structure.parent.raise_attribute_error (sage/structure/parent.c:2724) AttributeError: 'EllipticCurve_rational_field' object has no attribute '_EllipticCurve_rational_field__gens' **********************************************************************
The following tests failed: sage -t -long -force_lib devel/sage/doc/en/bordeaux_2008/elliptic_curves.rst # 7 doctests failed sage -t -long -force_lib devel/sage/sage/schemes/elliptic_curves/padics.py # 23 doctests failed sage -t -long -force_lib devel/sage/sage/schemes/elliptic_curves/ell_generic.py # 13 doctests failed sage -t -long -force_lib devel/sage/sage/schemes/elliptic_curves/padic_lseries.py # 2 doctests failed sage -t -long -force_lib devel/sage/sage/schemes/elliptic_curves/ell_point.py # 11 doctests failed sage -t -long -force_lib devel/sage/sage/schemes/elliptic_curves/ell_tate_curve.py # 4 doctests failed sage -t -long -force_lib devel/sage/sage/schemes/elliptic_curves/BSD.py # 4 doctests failed sage -t -long -force_lib devel/sage/sage/schemes/elliptic_curves/gp_simon.py # 1 doctests failed sage -t -long -force_lib devel/sage/sage/schemes/elliptic_curves/ell_number_field.py # 3 doctests failed sage -t -long -force_lib devel/sage/sage/schemes/elliptic_curves/heegner.py # 33 doctests failed sage -t -long -force_lib devel/sage/sage/schemes/elliptic_curves/sha_tate.py # 27 doctests failed sage -t -long -force_lib devel/sage/sage/schemes/elliptic_curves/ell_rational_field.py # 17 doctests failed
comment:7 Changed 9 years ago by
It would appear that I have made a serious error. I did run doctests, and they passed. I will admit to the possibility of having made a rookie mistake and tested the wrong copy of sage on my machine.
Another possible explanation is that I didn't have the large Cremona database installed when I applied the patch. So I applied the patch THEN installed the database and it seems the doctests passed under those circumstances.
I'm running to tests again to see what happened.
comment:8 Changed 9 years ago by
Jamie, let me take another look at this.
comment:9 Changed 9 years ago by
OK, fixed. There was a simple indentation error in the patch so removing 3 spaces did the trick. The patch did work OK on Sage+large database but not without the large database. Luckily I still had a 4.6.rc1 lying around without the database installed so I could test on that as well as on 4.7.alpha2 + database. (There's no way of uninstalling the database!)
I'm now doing full test of entire library with and without the database, and will post a new patch when done.
comment:10 Changed 9 years ago by
- Status changed from needs_work to needs_review
OK, please re-review this one. As I said, the difference between the previous patch and this one is just one indentation.
In order to test this one should test against a Sage installation with and without the spkg database_cremona_ellcurve. I have done that (full test of entire library).
comment:11 Changed 8 years ago by
- Cc mstreng added
- Status changed from needs_review to needs_work
sage 4.7 without database spkg without patch:
sage: E=EllipticCurve('389a1') sage: E.gens() [(-1 : 1 : 1), (0 : -1 : 1)]
sage 4.7 without database spkg with patch:
sage: E=EllipticCurve('389a1') sage: E.gens() ... AttributeError: 'EllipticCurve_rational_field' object has no attribute '_EllipticCurve_rational_field__gens'
comment:12 Changed 8 years ago by
- Reviewers changed from Jamie Weigandt to Jamie Weigandt, Marco Streng
- Status changed from needs_work to needs_review
I have fixed that problem (which was a real problem, not just a rebase). Please retest! I did test the whole library before and after installing the optional database.
comment:13 Changed 8 years ago by
- Status changed from needs_review to positive_review
comment:14 Changed 8 years ago by
- Merged in set to sage-4.7.1.alpha4
- Resolution set to fixed
- Status changed from positive_review to closed
I have found the problem, in lines 190-199 in ell_rational_field.py. It copies the points from the database curve in a way which does not change te curve those points lie on.
Patch on its way!