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 cremona)

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)

trac_10999-gens.patch (3.4 KB) - added by cremona 8 years ago.
Applies to 4.7.1.alpha2

Download all attachments as: .zip

Change History (15)

comment:1 Changed 9 years ago by cremona

  • 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 cremona

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!

comment:3 Changed 9 years ago by cremona

I'm testing the whole library now and will ask for reviews if that goes ok.

comment:4 Changed 9 years ago by cremona

  • Status changed from new to needs_review

comment:5 follow-up: Changed 9 years ago by weigandt

  • Authors set to John Cremona
  • 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 jdemeyer

  • 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 weigandt

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 cremona

Jamie, let me take another look at this.

comment:9 Changed 9 years ago by cremona

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 cremona

  • 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 mstreng

  • 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'

Changed 8 years ago by cremona

Applies to 4.7.1.alpha2

comment:12 Changed 8 years ago by cremona

  • 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 mstreng

  • Status changed from needs_review to positive_review

comment:14 Changed 8 years ago by jdemeyer

  • Merged in set to sage-4.7.1.alpha4
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.