Opened 10 years ago

Closed 6 years ago

#10665 closed defect (fixed)

bug in elliptic curve two_descent command

Reported by: was Owned by: cremona
Priority: minor Milestone: sage-6.3
Component: elliptic curves Keywords:
Cc: Merged in:
Authors: Peter Bruin Reviewers: Frédéric Chapoton
Report Upstream: N/A Work issues:
Branch: b437f35 (Commits) Commit: b437f3539dbda9bdb16c400d406925f3f2a20293
Dependencies: Stopgaps:


Don't do it twice:

sage: E = EllipticCurve([1,1,0,0,528])
sage: E.two_descent(verbose=False)
sage: E.two_descent(verbose=False)
RuntimeError                              Traceback (most recent call last)

/Users/wstein/<ipython console> in <module>()

/Users/wstein/sage/install/sage-4.6.1/local/lib/python2.6/site-packages/sage/schemes/elliptic_curves/ell_rational_field.pyc in two_descent(self, verbose, selmer_only, first_limit, second_limit, n_aux, second_descent)
    868                         n_aux, second_descent)
    869         if C.certain():
--> 870             self.__gens[True] = [self.point(x, check=True) for x in C.gens()]
    871             self.__gens[True].sort()
    872             self.__rank[True] = len(self.__gens[True])

/Users/wstein/sage/install/sage-4.6.1/local/lib/python2.6/site-packages/sage/libs/mwrank/interface.pyc in gens(self)
    598         from sage.misc.all import preparse
    599         from sage.rings.all import Integer
--> 600         return eval(preparse(self.__two_descent_data().getbasis().replace(":",",")))
    602     def certain(self):


Change History (10)

comment:1 Changed 10 years ago by weigandt

This is probably explained by Cremona's comment at #10108:

"I think I can explain this. The problem to be solved for this ticket was that if mwrank is given incomplete but otherwise correct input, it just waits for the rest of the input, making Sage appear to hang. To fix this I made sure that the input provided by Sage always has some other stuff appended to it, so mwrank never has insufficient input. But then, the *next* time input is sent to mwrank, there is likely to be still some of that extra stuff in its input buffer. To get around that (I thought) I made sure that mwrank was restarted at every call.

Clearly what I did was insufficient, but this does explain who the order of executing commands does matter."

comment:2 Changed 10 years ago by cremona

And there are patches at #10108 which represent a lot of work which have been lying about unused and unmerged just because they are not perfect, even if Sage is better with them than without.

comment:3 Changed 7 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:4 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:5 Changed 7 years ago by wuthrich

Even after #10108, this is still a problem !

comment:6 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:7 Changed 6 years ago by pbruin

The cause is that each time you call mwrank_EllipticCurve.two_descent(), it creates a new __descent (the 2-descent data) but does not reset the __saturate bound. Therefore no saturation is done on the new __descent data, which is bad because getbasis() depends on the saturation having been done. I'm now testing a patch.

comment:8 Changed 6 years ago by pbruin

  • Authors set to Peter Bruin
  • Branch set to u/pbruin/10665-mwrank_elliptic_curve_two_descent
  • Commit set to b437f3539dbda9bdb16c400d406925f3f2a20293
  • Status changed from new to needs_review

(Note: the change to if not self.__descent.ok() is just a trivial simplification, the important line is self.__saturate = -2.)

comment:9 Changed 6 years ago by chapoton

  • Reviewers set to Frédéric Chapoton
  • Status changed from needs_review to positive_review

ok, the patchbot is green, and the patch looks simple enough to me.

So let me give a positive review.

comment:10 Changed 6 years ago by vbraun

  • Branch changed from u/pbruin/10665-mwrank_elliptic_curve_two_descent to b437f3539dbda9bdb16c400d406925f3f2a20293
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.