enumerate_totallyreal_fields bug fix

[jvoight]

sage: enumerate_totallyreal_fields_all(8, 10^8)
---------------------------------------------------------------------------
IndexError                                Traceback (most recent call last)

/home/jvoight/sage-4.2/devel/sage-main/sage/rings/number_field/<ipython console> in <module>()

/usr/local/share/sage-5.0/local/lib/python2.7/site-packages/sage/rings/number_field/totallyreal_rel.pyc in enumerate_totallyreal_fields_all(n, B, verbose, return_seqs)
    887                     print "Taking F =", Sds[i][1]
    888                 F = NumberField(ZZx(Sds[i][1]), 't')
--> 889                 T = enumerate_totallyreal_fields_rel(F, n/d, B, verbose=verbose, return_seqs=return_seqs)
    890                 if return_seqs:
    891                     for i in range(3):

/usr/local/share/sage-5.0/local/lib/python2.7/site-packages/sage/rings/number_field/totallyreal_rel.pyc in enumerate_totallyreal_fields_rel(F, m, B, a, verbose, return_seqs)
    726         T.incr(f_out,verbose)
    727     else:
--> 728         T.incr(f_out)
    729 
    730     Fx = PolynomialRing(F, 'xF')

/usr/local/share/sage-5.0/local/lib/python2.7/site-packages/sage/rings/number_field/totallyreal_rel.pyc in incr(self, f_out, verbose, haltk)
    542                         # New bounds from Lagrange multiplier in degree 3.

    543                         bminmax = [lagrange_degree_3(m,v(self.a[m-1]),v(self.a[m-2]),v(self.a[m-3])) for v in self.Foo]
--> 544                         self.b_lower = [bminmax[i][0] for i in range(len(bminmax))]
    545                         self.b_upper = [bminmax[i][1] for i in range(len(bminmax))]
    546 

IndexError: list index out of range

The fix is easy: we just need to add the lines

    if len(z4minmax) < 1: 
        z4minmax = [0.0, -1.0]
    return z4minmax

before line 295 of sage/rings/number_field/totallyreal_data.pyx. (Yes, I'm still intimidated by creating a patch.)

The issue was that when the lagrange multipliers gives contradictory bounds (a good thing, since it says the enumeration can stop in that branch), it was not sending a pair of bounds, just a single element, causing a type error.

JV

Apply: attachment: trac_13101_bug_in_enumerate_totallyreal_fields_all.patch,
attachment: trac_13101_review.patch,
attachment: trac_13101_long_time.patch,

Component: number fields

Keywords: sd51

Author: Robert Harron, John Voight, Frédéric Chapoton

Branch/Commit: u/vbraun/enumerate_totallyreal @ 3923f3a

Reviewer: Alex Ghitza, Peter Bruin

Issue created by migration from https://trac.sagemath.org/ticket/13101

[rharron]

comment:1

Attachment: trac_13101_bug_in_enumerate_totallyreal_fields_all.patch.gz

Okay, I made a patch with the change you mentioned (I think I figured out what indentation you meant, but double-check to make sure). The output of this command should be the empty array, right? That's what I gather from the data on your website/your paper.

(Also, I fixed a typo).

[rharron]

Changed reviewer from citro to none

[rharron]

Author: Robert Harron, John Voight

[jvoight]

comment:3

Thanks for doing this and for fixing the typo! Yes, the tabbing/formatting is correct. Am I allowed to review this? JV

[rharron]

comment:4

Hmm, I don't really know how that works. I mean, if you did, you'd probably want to remove your name from the authors... You could try, what's the worse that could happen?!

[aghitza]

Changed keywords from none to sd51

[aghitza]

Reviewer: Alex Ghitza

[jdemeyer]

comment:7

The added doctest really takes too long. Can the bug be tested with a shorted test? Otherwise, simply mark the test something like

# not tested (too long time)

[jvoight]

comment:8

Did someone add

sage: enumerate_totallyreal_fields_all(8, 10^8)

to the doctest? This was just an example that caused the stupid bug and I don't think it was ever intended for a doctest. But then we also probably want to keep it for future testing purposes, right?

There is an easy doctest in there already

enumerate_totallyreal_fields_rel(F, 2, 2000)

and for general purposes of testing, this should be enough, IMHO. JV

[rharron]

comment:9

Yeah, I added that doctest: every time there is a bug fix, there has to be a corresponding doctest added to show that the bug has been fixed. Since I'm not really familiar with the workings of this part of sage, I simply took the snippet of code from the bug report and added it as a doctest. What Jeroen is asking is whether there is a shorter test that would fail before this bug fix but pass afterwards, thus showing things were fixed. Can you cook one of those up?

Replying to @jvoight:

Did someone add

sage: enumerate_totallyreal_fields_all(8, 10^8)

to the doctest? This was just an example that caused the stupid bug and I don't think it was ever intended for a doctest. But then we also probably want to keep it for future testing purposes, right?

There is an easy doctest in there already

enumerate_totallyreal_fields_rel(F, 2, 2000)

and for general purposes of testing, this should be enough, IMHO. JV

[jvoight]

comment:10

OK, got it. Then yes, just use

sage: enumerate_totallyreal_fields_all(8, 10^6)

which should return

[]

as the smallest totally real octic field has discriminant 282300416 > 10⁶. This triggers the error on my unpatched version. Is this quick enough for the doctest?

JV

[fchapoton]

comment:11

Attachment: trac_13101_review.patch.gz

here is a review patch, with more reasonable tests.

[pjbruin]

Changed author from Robert Harron, John Voight to Robert Harron, John Voight, Frédéric Chapoton

[pjbruin]

Changed reviewer from Alex Ghitza to Alex Ghitza, Peter Bruin

[pjbruin]

comment:12

Looks good. The new doctest takes around 2 seconds on the AMD64 2.2 GHz system that I tested it on. I think marking this as "long time" is still justified; I'm adding this as a reviewer patch.

[pjbruin]

mark doctest as "long time"

[vbraun]

Branch: u/vbraun/enumerate_totallyreal

[vbraun]

Commit: 3923f3a

[vbraun]

comment:13

Attachment: trac_13101_long_time.patch.gz

New commits:

3923f3a	Trac 13101: mark doctest as "long time"
a807b94	trac 13101 better doctest
cfd405b	Trac 13101: Fix bug in enumerate_totallyreal_fields_all