Opened 12 years ago

Closed 10 years ago

Last modified 10 years ago

#6329 closed defect (fixed)

optional doctest failure -- breakage in the sage<-->magma interface

Reported by: was Owned by: tbd
Priority: minor Milestone: sage-4.7.2
Component: interfaces Keywords: sd32
Cc: Merged in: sage-4.7.2.alpha3
Authors: Mariah Lenox, William Stein Reviewers: Mike Hansen
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by leif)

sage -t -long --optional devel/sage/sage/rings/rational.pyx
**********************************************************************
File "/scratch/wstein/build/sage-4.0.2.alpha3/devel/sage-main/sage/rings/rational.pyx", line 3087:
    sage: magma(3/1).Type()             # optional
Expected:
    FldRatElt
Got:
    RngIntElt
**********************************************************************
1 items had failures:
   1 of   5 in __main__.example_84
***Test Failed*** 1 failures.

Apply only trac_6329.2.patch to the Sage library.

Attachments (2)

trac_6329.patch (993 bytes) - added by mariah 11 years ago.
trac_6329.2.patch (812 bytes) - added by was 10 years ago.
apply only this one

Download all attachments as: .zip

Change History (16)

comment:1 Changed 12 years ago by was

  • Summary changed from optional doctest failure -- bad data type breakage in the sage-->magma interface to optional doctest failure -- breakage in the sage<-->magma interface

More failures:

sage -t -long --optional devel/sage/sage/schemes/hyperelliptic_curves/hyperelliptic_g2_generic.py
**********************************************************************
File "/scratch/wstein/build/sage-4.0.2.alpha3/devel/sage-main/sage/schemes/hyperelliptic_curves/hyperelliptic_g2_generic.py", line 218:
    sage: magma(HyperellipticCurve(f)).IgusaClebschInvariants() # optional - magma
Expected:
    [ 0, -2048/375, -4096/25, -4881645568/84375 ]
Got:
    [ -640, -20480, 1310720, 52160364544 ]
**********************************************************************
File "/scratch/wstein/build/sage-4.0.2.alpha3/devel/sage-main/sage/schemes/hyperelliptic_curves/hyperelliptic_g2_generic.py", line 220:
    sage: magma(HyperellipticCurve(f(2*x))).IgusaClebschInvariants() # optional - magma
Expected:
    [ 0, -8388608/375, -1073741824/25, -5241627016305836032/84375 ]
Got:
    [ -40960, -83886080, 343597383680, 56006764965979488256 ]
**********************************************************************
File "/scratch/wstein/build/sage-4.0.2.alpha3/devel/sage-main/sage/schemes/hyperelliptic_curves/hyperelliptic_g2_generic.py", line 222:
    sage: magma(HyperellipticCurve(f, x)).IgusaClebschInvariants() # optional - magma
Expected:
    [ -8/15, 17504/5625, -23162896/140625, -420832861216768/7119140625 ]
Got:
    [ -640, 17920, -1966656, 52409511936 ]
**********************************************************************
File "/scratch/wstein/build/sage-4.0.2.alpha3/devel/sage-main/sage/schemes/hyperelliptic_curves/hyperelliptic_g2_generic.py", line 224:
    sage: magma(HyperellipticCurve(f(2*x), 2*x)).IgusaClebschInvariants() # optional - magma
Expected:
    [ -512/15, 71696384/5625, -6072014209024/140625, -451865844002031331704832/7119140625 ]
Got:
    [ -40960, 73400320, -515547070464, 56274284941110411264 ]
**********************************************************************
1 items had failures:
   4 of  12 in __main__.example_7
***Test Failed*** 4 failures.

comment:2 Changed 12 years ago by was

More failures:

sage -t -long --optional devel/sage/sage/interfaces/magma.py
**********************************************************************
File "/scratch/wstein/build/sage-4.0.2.alpha3/devel/sage-main/sage/interfaces/magma.py", line 856:
    sage: magma.attach('%s/data/extcode/magma/sage/basic.m'%Sage_ROOT)    # optional - magma
Exception raised:
    Traceback (most recent call last):
      File "/scratch/wstein/build/sage-4.0.2.alpha3/local/bin/ncadoctest.py", line 1231, in run_one_test
        self.run_one_example(test, example, filename, compileflags)
      File "/scratch/wstein/build/sage-4.0.2.alpha3/local/bin/sagedoctest.py", line 38, in run_one_example
        OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
      File "/scratch/wstein/build/sage-4.0.2.alpha3/local/bin/ncadoctest.py", line 1172, in run_one_example
        compileflags, 1) in test.globs
      File "<doctest __main__.example_20[2]>", line 1, in <module>
        magma.attach('%s/data/extcode/magma/sage/basic.m'%Sage_ROOT)    # optional - magma###line 856:
    sage: magma.attach('%s/data/extcode/magma/sage/basic.m'%Sage_ROOT)    # optional - magma
    NameError: name 'Sage_ROOT' is not defined
**********************************************************************
File "/scratch/wstein/build/sage-4.0.2.alpha3/devel/sage-main/sage/interfaces/magma.py", line 860:
    sage: magma.attach('%s/data/extcode/magma/sage/basic2.m'%Sage_ROOT)     # optional - magma
Expected:
    Traceback (most recent call last):
    ...
    RuntimeError: Error evaluating Magma code...
Got:
    Traceback (most recent call last):
      File "/scratch/wstein/build/sage-4.0.2.alpha3/local/bin/ncadoctest.py", line 1231, in run_one_test
        self.run_one_example(test, example, filename, compileflags)
      File "/scratch/wstein/build/sage-4.0.2.alpha3/local/bin/sagedoctest.py", line 38, in run_one_example
        OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
      File "/scratch/wstein/build/sage-4.0.2.alpha3/local/bin/ncadoctest.py", line 1172, in run_one_example
        compileflags, 1) in test.globs
      File "<doctest __main__.example_20[3]>", line 1, in <module>
        magma.attach('%s/data/extcode/magma/sage/basic2.m'%Sage_ROOT)     # optional - magma###line 860:
    sage: magma.attach('%s/data/extcode/magma/sage/basic2.m'%Sage_ROOT)     # optional - magma
    NameError: name 'Sage_ROOT' is not defined
**********************************************************************
File "/scratch/wstein/build/sage-4.0.2.alpha3/devel/sage-main/sage/interfaces/magma.py", line 917:
    sage: print magma.load(SAGE_TMP + 'a.m')      # optional - magma
Expected:
    Loading ".../.sage//temp/.../a.m"
    hi
Got:
    Loading "/scratch/wstein/sage//temp/sage.math.washington.edu/31604/a.m"
    hi
**********************************************************************
File "/scratch/wstein/build/sage-4.0.2.alpha3/devel/sage-main/sage/interfaces/magma.py", line 2148:
    sage: magma.eval('R<x> := PolynomialRing(RationalField()); f := (x-17/2)^3;')     # optional - magma
Expected:
    "
Got:
    ''
**********************************************************************
File "/scratch/wstein/build/sage-4.0.2.alpha3/devel/sage-main/sage/interfaces/magma.py", line 2160:
    sage: magma.eval('K<a> := CyclotomicField(11)')       # optional - magma
Expected:
    "
Got:
    ''
**********************************************************************
File "/scratch/wstein/build/sage-4.0.2.alpha3/devel/sage-main/sage/interfaces/magma.py", line 463:
    sage: magma.eval("a := %s;"%(10^10000))    # optional - magma
Expected:
    "
Got:
    ''
**********************************************************************
4 items had failures:
   2 of   4 in __main__.example_20
   1 of   5 in __main__.example_22
   2 of  28 in __main__.example_64
   1 of   4 in __main__.example_9
***Test Failed*** 6 failures.
For whitespace errors, see the file /home/wstein/build/sage-4.0.2.alpha3/tmp/.doctest_magma.py
	 [32.9 s]

Changed 11 years ago by mariah

comment:3 Changed 11 years ago by mariah

  • Authors set to Mariah Lenox
  • Milestone changed from sage-4.7 to sage-4.7.1
  • Priority changed from major to minor
  • Report Upstream set to N/A
  • Status changed from new to needs_review

trac_6329.patch corrects the test output in the problem in the description. The Got output is correct - there is not a breakage in the sage<-->magma interface. 'magma(3/1)' returns the integer '3' (after coercion), so Type() returns that the value is an integer.

The other two reports of failures are no longer valid, as they seem to already be fixed in sage-4.7.rc4.

comment:4 Changed 10 years ago by kcrisman

  • Component changed from optional packages to interfaces

comment:5 Changed 10 years ago by mhansen

  • Status changed from needs_review to needs_info

It seems like the correct fix would be to always put the denominator in the _magma_init_ method.

        s = self.numerator()._magma_init_(magma)
        s += '/' + self.denominator()._magma_init_(magma)
        return s

instead of

        s = self.numerator()._magma_init_(magma)
        if not self.is_integral():
            s += '/' + self.denominator()._magma_init_(magma)
        return s

comment:6 Changed 10 years ago by was

Mhansen, you're right, the patch Mariah posted is wrong. I'll post a correct patch, and hopefully you can review it.

Changed 10 years ago by was

apply only this one

comment:7 Changed 10 years ago by was

  • Status changed from needs_info to needs_review

Patch up that is ready for review (the .2 one). This was a result of some overzealous optimization on my part a few years ago. Reverting this change will barely slow things down.

comment:8 follow-up: Changed 10 years ago by mhansen

We should add a doctest for the integral case.

comment:9 in reply to: ↑ 8 Changed 10 years ago by was

Replying to mhansen:

We should add a doctest for the integral case.

What do you mean by this? If you mean sage: magma(3/1).Type(), which is in the ticket description, then there is *already* a doctest for this case, which is how we found this bug in the first place.

Or do you mean adding something to integer.pyx???

comment:10 Changed 10 years ago by was

  • Keywords sd32 added

comment:11 Changed 10 years ago by mhansen

  • Status changed from needs_review to positive_review

I was thinking about the old code and verifying that the "is_integral" code path worked (since that test was missing before). However, it is obviously not needed now.

Positive review.

comment:12 Changed 10 years ago by leif

  • Authors changed from Mariah Lenox to Mariah Lenox, William Stein
  • Description modified (diff)
  • Reviewers set to Mike Hansen

comment:13 Changed 10 years ago by leif

  • Merged in set to sage-4.7.2.alpha3
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:14 Changed 10 years ago by was

Shame on the idiot who wrote this with no tests... and who forgot that there are optional tests all over Sage that would be broken by this: see #12006, where I'm doing my penance.

Note: See TracTickets for help on using tickets.