Ticket #4287 (closed enhancement: fixed)

Opened 2 months ago

Last modified 1 week ago

[with patch, positive review] improve elliptic curve doctest (part 5)

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

Description

This is for the file formal_group.py. Note that adding a s == loads(dumps(s)) test revealed a failure: 

sage: E = EllipticCurve('11a')
sage: F = E.formal_group()
sage: F == loads(dumps(F))
False

Attachments

trac_4287.patch (6.3 kB) - added by zimmerma on 10/14/2008 01:00:19 PM.
trac_4287_2.patch (1.0 kB) - added by cremona on 10/19/2008 02:02:46 PM.

Change History

10/14/2008 01:00:19 PM changed by zimmerma

  • attachment trac_4287.patch added.

(follow-up: ↓ 2 ) 10/19/2008 01:02:25 PM changed by cremona

  • summary changed from [with patch, needs review] improve elliptic curve doctest (part 5) to [with patch, with review, needs work] improve elliptic curve doctest (part 5).

Patch looks good and applies ok. But as Paul says, we need to see why load(dumps(F))!=F for a formal group F.

I don't know how to fix this.

10/19/2008 02:02:46 PM changed by cremona

  • attachment trac_4287_2.patch added.

(in reply to: ↑ 1 ) 10/19/2008 02:03:19 PM changed by cremona

  • summary changed from [with patch, with review, needs work] improve elliptic curve doctest (part 5) to [with new patch, needs review] improve elliptic curve doctest (part 5).

Replying to cremona:

Patch looks good and applies ok. But as Paul says, we need to see why load(dumps(F))!=F for a formal group F. I don't know how to fix this.

I didn't know, but now I do. There was nothing wrong with loads() or dumps() for formal groups, but they were missing a _cmp_ function so the "==" comparison was not giving the expected answer. The second patch (modelled on a similar one by robertwb for ell_tate_curve.py) seems to do the trick. All tests pass in elliptic_curves.

10/21/2008 01:13:15 PM changed by cremona

  • cc set to robertwb.

Robert (robertwb), I CC'd you on this hoping that you could say that I fixed this appropriately? I was using a similar patch of yours as a model. John

10/22/2008 07:51:56 AM changed by robertwb

Yes, that looks good to me. (As does the other patch--though I only read it at SD 10, I didn't actually try it out yet).

11/21/2008 11:40:48 PM changed by AlexGhitza

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

The two patches look good and apply cleanly against my 3.2.

11/22/2008 11:57:00 PM changed by mabshoff

  • status changed from new to closed.
  • resolution set to fixed.
  • summary changed from [with new patch, positive review] improve elliptic curve doctest (part 5) to [with patch, positive review] improve elliptic curve doctest (part 5).

Merged in Sage 3.2.1.alpha0