Opened 12 years ago

Closed 12 years ago

Last modified 10 years ago

#4715 closed defect (fixed)

[with patch; positive review] Two small bugs in KodairaSymbol

Reported by: cremona Owned by: was
Priority: major Milestone: sage-3.2.2
Component: number theory Keywords:
Cc: tnagel Merged in:
Authors: Reviewers:
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

#4412 had a buglet: for Kodaira Class Im the _roman field was not being set (it should be 1). This is only currently used in the tamagawa_exponent() function for elliptic curves over number fields.

One-line patch coming up, plus a corresponding doctest.

This was reported by Tobias Nagel:

sage: E=EllipticCurve('117a3');                        
sage: E.tamagawa_exponent(13)
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)

/home/tobi/test_Sint/<ipython console> in <module>()

/home/tobi/sage/local/lib/python2.5/site-packages/sage/schemes/elliptic_curves/ell_rational_field.pyc in tamagawa_exponent(self, p)
 2190             return cp
 2191         ks = self.kodaira_type(p)
-> 2192         if ks._roman==1 and ks._n%2==0 and ks._starred:
 2193             return 2
 2194         return 4

AttributeError: 'KodairaSymbol_class' object has no attribute '_roman'

Attachments (2)

sage-trac-4715.patch (1.3 KB) - added by cremona 12 years ago.
sage-trac-4715-2.patch (4.7 KB) - added by cremona 12 years ago.

Download all attachments as: .zip

Change History (10)

Changed 12 years ago by cremona

comment:1 Changed 12 years ago by mabshoff

  • Cc tnagel added
  • Summary changed from Small bug in KodairaSymbol to [with patch, needs review] Small bug in KodairaSymbol

comment:2 Changed 12 years ago by mabshoff

  • Milestone set to sage-3.2.2

comment:3 Changed 12 years ago by cremona

  • Summary changed from [with patch, needs review] Small bug in KodairaSymbol to [with patch, not ready for review] Small bug in KodairaSymbol

There's another problem, watch this space:

sage: E=EllipticCurve('153c2')
sage: E.tamagawa_exponent(3)
<boom>

Changed 12 years ago by cremona

comment:4 Changed 12 years ago by cremona

  • Summary changed from [with patch, not ready for review] Small bug in KodairaSymbol to [with patch, needs review] Two small bugs in KodairaSymbol

Fixing that also showed up the following completely independent bug (only on 32-bit machines though):

sage: E=EllipticCurve('903b3')
sage: E.pari_curve()
<boom> (PariError: precision too low)

The second patch fixes that as well as the other (which only applied to type I*0). Now I have checked tamagawa_index() for all curves in the database up to conductor 10000 and all bad primes for each, so I hope that's that.

To fix the pari precision problem I did a try/except which keeps doubling the precision until it's ok. I hope that is not against the rules: if pari's ellinit every crashes for a reason other than precision, this would be an infinite loop.

In the course of this testing I found that looping through thousands of curves ate up a lot of memory. I made a change so that for curves over QQ, local_data() uses prime integers arther than prime ideals, and that helps a bit, but there is still more memory begin eaten up than I would like. For example:

sage: for e in cremona_curves(srange(11,10000)):
    for p in e.conductor().support():
        ld = e.local_data(p)
        print e.cremona_label()   

On my machine the used RAM creeps up gradually, hits 1GB at around conductor 2400, and if I let it continue it starts to make my machine really suffer at 1.7GB (no prizes for guessing the amount of RAM I have).

This might deserve a separate ticket.

comment:5 Changed 12 years ago by mabshoff

Hi John,

please open a ticket for the memory issues/leaks you are seeing. We are current chasing a number of leaks, most of which seem coercion related.

Cheers,

Michael

comment:6 Changed 12 years ago by was

  • Summary changed from [with patch, needs review] Two small bugs in KodairaSymbol to [with patch; positive review] Two small bugs in KodairaSymbol

comment:7 Changed 12 years ago by mabshoff

  • Resolution set to fixed
  • Status changed from new to closed

Merged both patches in Sage 3.2.2.alpha1

comment:8 Changed 10 years ago by jdemeyer

  • Report Upstream set to N/A

In #9931, I plan to revert this ticket's patch to sage/schemes/elliptic_curves/ell_rational_field.py, since the workaround seems not needed anymore.

Note: See TracTickets for help on using tickets.