Ticket #4281 (closed enhancement: fixed)

Opened 2 months ago

Last modified 1 month ago

[with patch,with positive review] elliptic curve doctest coverage (part 4)

Reported by: zimmerma Assigned to: was
Priority: minor Milestone: sage-3.2
Component: algebraic geometry Keywords:
Cc:

Description

This is for the file ell_tate_curve.py. I was unable to doctest the _height function, which is used as a closure. Also, the missing loads(dumps(..)) test fails: 

sage: e = EllipticCurve('130a1')
sage: eq = e.tate_curve(5)
sage: eq == loads(dumps(eq))
False

Attachments

trac_4281.patch (3.4 kB) - added by zimmerma on 10/14/2008 06:49:50 AM.
trac_4281.patch2 (4.0 kB) - added by zimmerma on 10/14/2008 07:00:49 AM.
4281-tate-pickle.patch (0.9 kB) - added by robertwb on 10/18/2008 10:02:24 AM.

Change History

10/14/2008 06:49:50 AM changed by zimmerma

  • attachment trac_4281.patch added.

10/14/2008 07:00:49 AM changed by zimmerma

  • attachment trac_4281.patch2 added.

10/14/2008 07:01:52 AM changed by zimmerma

The second patch fixed a few typos (to be applied after the 1st one).

10/14/2008 02:51:40 PM changed by robertwb

The attached patch fixes the loads/dumps issue.

10/15/2008 03:00:00 AM changed by wuthrich

The last patch does not for fix it for me. Do I do something wrong ? (This is the same patch as for #4289.)

10/18/2008 04:30:46 AM changed by zimmerma

I agree with Chris. Probably Robert attached the wrong patch. This one (4281-tate-pickle.patch) is already in 3.1.4 but the loads/dumps problem is still there: 

----------------------------------------------------------------------
| SAGE Version 3.1.4, Release Date: 2008-10-16                       |
| Type notebook() for the GUI, and license() for information.        |
----------------------------------------------------------------------

sage: e = EllipticCurve('130a1')
sage: eq = e.tate_curve(5)
sage: eq == loads(dumps(eq))
False

10/18/2008 10:02:24 AM changed by robertwb

  • attachment 4281-tate-pickle.patch added.

10/18/2008 10:02:45 AM changed by robertwb

Yep, sorry, I posted the wrong patch. I've replaced it now.

10/18/2008 10:51:11 AM changed by zimmerma

Robert's new patch is ok, thus I give a positive review for it. However I cannot review my own patches...

10/19/2008 01:43:57 PM changed by cremona

  • summary changed from [with patch, needs review] elliptic curve doctest coverage (part 4) to [with patch,with positive review] elliptic curve doctest coverage (part 4).

N.B. To apply the second patch properly I had to rename it to trac_4281_2.patch: applying the patch failed when the suffix was patch2.

Apart from that, the sequence of patches applies fine and all tests in elliptic_curves pass.

I also learnt from robertwb's patch one way in which loads(dumps(*)) can fail, so will return to the other ticket...

(follow-up: ↓ 9 ) 10/20/2008 02:37:26 AM changed by wuthrich

Strictly speaking there is still something to do. It checks if E and p are equal. In a perfect implementation this should be an elliptic curve over a local field and we should check if they are isomorphic over this field, not over Q.

But I agree that the patch fixes this by now and the ticket should be closed.

(in reply to: ↑ 8 ) 10/20/2008 02:44:46 AM changed by cremona

Replying to wuthrich:

Strictly speaking there is still something to do. It checks if E and p are equal. In a perfect implementation this should be an elliptic curve over a local field and we should check if they are isomorphic over this field, not over Q.

When we have a type to hold elliptic curves over local fields then this can perhaps be changed. I also did not bother to compare the (possibly) cached power series which are part of the class's data. As I see it, this _cmp_ function is only there for technical Pythonic reasons, and serious mathematical functionality would not be implemented via the == operator.

But I agree that the patch fixes this by now and the ticket should be closed.

Good! Thanks.

10/20/2008 07:01:54 AM changed by mabshoff

  • status changed from new to closed.
  • resolution set to fixed.

Merged all three patches in Sage 3.2.alpha0