Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dozens of failures in magma optional test suite on skynet (eno) with sage-4.3 #7870

Closed
williamstein opened this issue Jan 7, 2010 · 52 comments

Comments

@williamstein
Copy link
Contributor

I ran the magma optional test suite on skynet (eno):

./sage -t --only_optional=magma devel/sage/sage > magma.out&

And the failures are:

        sage -t --only_optional=magma "devel/sage/sage/rings/polynomial/pbori.pyx"
        sage -t --only_optional=magma "devel/sage/sage/rings/polynomial/multi_polynomial_ring_generic.pyx"
        sage -t --only_optional=magma "devel/sage/sage/rings/polynomial/term_order.py"
        sage -t --only_optional=magma "devel/sage/sage/rings/polynomial/multi_polynomial_ideal.py"
        sage -t --only_optional=magma "devel/sage/sage/crypto/mq/mpolynomialsystem.py"
        sage -t --only_optional=magma "devel/sage/sage/schemes/hyperelliptic_curves/hyperelliptic_g2_generic.py"
        sage -t --only_optional=magma "devel/sage/sage/symbolic/expression.pyx"
Total time for all tests: 364.0 seconds

Apply:

  1. attachment: trac-7870-magma-doctest-review3.patch
  2. attachment: trac-7870-magma-doctest-review4.patch

Component: interfaces

Keywords: Magma

Author: William Stein, John Cremona

Reviewer: Martin Raum, John Cremona, Jeroen Demeyer

Merged: sage-4.7.alpha2

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

@williamstein williamstein added this to the sage-4.7 milestone Jan 7, 2010
@williamstein williamstein self-assigned this Jan 7, 2010
@williamstein
Copy link
Contributor Author

this is the output of running the test suite, showing the actual failures.

@williamstein
Copy link
Contributor Author

comment:1

Attachment: magma.out.gz

I tested again using the new magma V2.16-7 with sage-4.3.5.

[wstein@eno sage-4.3.5]$ magma
Magma V2.16-7     Tue Apr  6 2010 11:14:18 on eno      [Seed = 3125460604]
Type ? for help.  Type <Ctrl>-D to quit.

There are now even more failures. I've attached a new test log created using the following on eno:

./sage -tp 10 -only_optional=magma devel/sage/sage

@williamstein
Copy link
Contributor Author

it got much worse!

@williamstein
Copy link
Contributor Author

Attachment: magma-sage-4.3.5.out.gz

Attachment: trac_7870.patch.gz

This fixes all the doctest issues on eno with magma V2.16-7

@JohnCremona
Copy link
Member

comment:3

I am testing this now with magma V2.16-1 and will report back. It will not be a clean result, since I already saw


**********************************************************************
File "/storage/jec/sage-4.3.5/devel/sage-tests/sage/symbolic/expression.pyx", line 499:
    sage: magma(f)                         # optional - magma
Expected:
    sin(cos(x^2) + log(x))
Got:
    sin(log(x) + cos(x^2))

@JohnCremona
Copy link
Member

comment:4

Full log is at http://www.warwick.ac.uk/staff/J.E.Cremona/magma_test.log .

@JohnCremona
Copy link
Member

comment:5

Replying to @JohnCremona:

Full log is at http://www.warwick.ac.uk/staff/J.E.Cremona/magma_test.log .

Apologies -- it looks as if I did not apply the patch since the differences look exactly like the ones you fixed! I will try again.

@JohnCremona
Copy link
Member

Author: William Stein

@JohnCremona
Copy link
Member

Reviewer: John Cremona

@JohnCremona
Copy link
Member

comment:6

OK, so after actually applying the patch (I had forgotten to do hg qpush) I get exactly one failure. This is on 64-bit ubuntu, Sage 4.3.5 and Magma V2.16-1:

sage -t --only_optional=magma "./sage/interfaces/magma.py"  
**********************************************************************
File "/storage/jec/sage-4.3.5/devel/sage-tests/sage/interfaces/magma.py", line 187:
    sage: y * 1.0                                                         # optional - magma
Exception raised:
    Traceback (most recent call last):
      File "/home/jec/sage-current/local/bin/ncadoctest.py", line 1231, in run_one_test
        self.run_one_example(test, example, filename, compileflags)
      File "/home/jec/sage-current/local/bin/sagedoctest.py", line 38, in run_one_example
        OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
      File "/home/jec/sage-current/local/bin/ncadoctest.py", line 1172, in run_one_example
        compileflags, 1) in test.globs
      File "<doctest __main__.example_0[46]>", line 1, in <module>
        y * RealNumber('1.0')                                                         # optional - magma###line 187:
    sage: y * 1.0                                                         # optional - magma
      File "element.pyx", line 1398, in sage.structure.element.RingElement.__mul__ (sage/structure/element.c:11337)
        return coercion_model.bin_op(left, right, mul)
      File "coerce.pyx", line 717, in sage.structure.coerce.CoercionModel_cache_maps.bin_op (sage/structure/coerce.c:6212)
        raise
      File "coerce.pyx", line 713, in sage.structure.coerce.CoercionModel_cache_maps.bin_op (sage/structure/coerce.c:6151)
        return PyObject_CallObject(op, xy)
      File "element.pyx", line 1393, in sage.structure.element.RingElement.__mul__ (sage/structure/element.c:11265)
        return (<RingElement>left)._mul_(<RingElement>right)
      File "element.pyx", line 1400, in sage.structure.element.RingElement._mul_ (sage/structure/element.c:11385)
        cpdef RingElement _mul_(self, RingElement right):
      File "/home/jec/sage-current/local/lib/python/site-packages/sage/interfaces/expect.py", line 1909, in _mul_
        return self._operation('*', right)
      File "/home/jec/sage-current/local/lib/python/site-packages/sage/interfaces/expect.py", line 1866, in _operation
        raise TypeError, msg
    TypeError: Error evaluating Magma code.
    IN:
[27]:=_sage_[19] * _sage_[25];
    OUT:
    >> _sage_[27]:=_sage_[19] * _sage_[25];
                              ^
    Runtime error in '*': Bad argument types
    Argument types given: RngUPolElt[RngInt], FldReElt

and this looks like some error in parsing the expected output (are you allowed two different "..."?) since it looks fine to me. The only other things I can think of is that there may be different numbers of spaces in the expected and actual magma output!

@JohnCremona
Copy link
Member

Changed keywords from none to Magma

@williamstein
Copy link
Contributor Author

comment:7

John,

I think you should give my patch a positive review anyways. The problem above is that in Magma V2.16-7, this works fine:

[wstein@eno ~]$ magma
Magma V2.16-7     Mon Apr 26 2010 22:51:34 on eno      [Seed = 294390646]
Type ? for help.  Type <Ctrl>-D to quit.
> R<x> := PolynomialRing(Integers());
> x*1.0;
$.1
> 

However, in older versions of Magma, it doesn't:

flat:~ wstein$ magma
Magma V2.15-11    Mon Apr 26 2010 19:53:21 on flat     [Seed = 4201111680]
Type ? for help.  Type <Ctrl>-D to quit.
> R<x> := PolynomialRing(Integers());
> x*1.0;

>> x*1.0;
    ^
Runtime error in '*': Bad argument types
Argument types given: RngUPolElt[RngInt], FldReElt

Since Magma's capabilities, etc., change a lot -- even from minor version to version -- I think the Sage optional doctests should be targeted at the latest released version of Magma.

Note that the computation is multiplying a polynomial over ZZ[x] by a floating point numbers. In Sage, there is a beautiful coercion model that makes most such things "just work". In Magma, one implements the '*' function for every conceivable choice of pairs of types... and I guess somebody got around to eventually implementing this one.

Just to emphasize how totally arbitrary (and sad) Magma's system still is after all these years, notice that even in Magma V2.16-7, the same computation with polynomials over ZZ and rational numbers doesn't work!

> x + 1/2;        

>> x + 1/2;
     ^
Runtime error in '+': Bad argument types
Argument types given: RngUPolElt[RngInt], FldRatElt

> x*(1/2);

>> x*(1/2);
    ^
Runtime error in '*': Bad argument types
Argument types given: RngUPolElt[RngInt], FldRatElt

> 

Sage had the same sort of silly anomalies until people like David Harvey, Craig Citro, David Roe, and Robert Bradshaw and others stepped in and greatly improved the situation.

sage: R.<x> = ZZ[]
sage: x * 1.0
x
sage: parent(x * 1.0)
Univariate Polynomial Ring in x over Real Field with 53 bits of precision
sage: x + 1/2
x + 1/2
sage: (1/2)*x
1/2*x

Sage coercion is still of course far from perfect. But it's also far from sucking.

-- William

@JohnCremona
Copy link
Member

comment:9

I just lost my posting here (cookie problem) and don't feel like rewriting it all....

I only have C2.16-1 and the patch requires V2.16-7 or higher to pass, it seems, so I cannot verify it at present.

Is it anywhere documented which version of Magma Sage relies on for positive tests?

@JohnCremona
Copy link
Member

comment:10

With Sage 4.4 and a newly installed magma V1.16-7 I still get falures in these files:

	sage -t --only_optional=magma "devel/sage/sage/rings/finite_rings/element_givaro.pyx"
	sage -t --only_optional=magma "devel/sage/sage/rings/polynomial/multi_polynomial.pyx"
	sage -t --only_optional=magma "devel/sage/sage/rings/polynomial/polynomial_element.pyx"
	sage -t --only_optional=magma "devel/sage/sage/rings/polynomial/polynomial_ring.py"
	sage -t --only_optional=magma "devel/sage/sage/schemes/elliptic_curves/ell_generic.py"
	sage -t --only_optional=magma "devel/sage/sage/schemes/hyperelliptic_curves/hyperelliptic_generic.py"

See http://www.warwick.ac.uk/staff/J.E.Cremona/magma.out for the full log.

@sagetrac-mraum
Copy link
Mannequin

sagetrac-mraum mannequin commented Mar 9, 2011

comment:26

This seems right. So, if everything is ok for you, also, give it a positive review.

@sagetrac-drkirkby
Copy link
Mannequin

sagetrac-drkirkby mannequin commented Mar 9, 2011

comment:27

According to the sage -h, the option should be -only-optional=magma and not --only-optional=magma

I don't have Magma, but do have Mathematica, so I thought I'd try

drkirkby@hawk:~/sage-4.6.2.rc1$ ./sage -t -only_optional=mathematica devel/sage/sage
sage -t -only_optional=mathematica "devel/sage/sage/probability/all.py"
	 [0.1 s]
sage -t -only_optional=mathematica "devel/sage/sage/probability/__init__.py"
	 [0.1 s]
sage -t -only_optional=mathematica "devel/sage/sage/probability/random_variable.py"

but it seems to run every doctest, not just the Mathematica ones, which fail anyway, as noted at #8495.

Dave

@jdemeyer
Copy link

comment:28

This needs to be rebased to sage-4.7.alpha1.

@sagetrac-mraum
Copy link
Mannequin

sagetrac-mraum mannequin commented Mar 12, 2011

Attachment: trac-7870-magma-doctest-review3.patch.gz

@sagetrac-mraum

This comment has been minimized.

@sagetrac-mraum
Copy link
Mannequin

sagetrac-mraum mannequin commented Mar 12, 2011

comment:29

I rebased the patch to 4.7alpha1. Please only apply the last one: review3.

Could anyone (John?) check this soon, so that we won't need to rebase it again?

@JohnCremona
Copy link
Member

comment:30

Replying to @sagetrac-mraum:

I rebased the patch to 4.7alpha1. Please only apply the last one: review3.

Thanks.

Could anyone (John?) check this soon, so that we won't need to rebase it again?

OK, I'll try that soon. (I have just been away for the weekend or I would have done it already.)

@JohnCremona
Copy link
Member

comment:31

With magma-V2.17-5 (which I downloaded and installed specially) and sage-4.7.alpha1 I tested everything both with and without -only-optional=magma and in both cases all tests passed.

So I am giving this a positive review (again).

@jdemeyer
Copy link

comment:32

It seems that your 'review3' patch is based on an older version of the patches, I get the number_field doctest failures again.

@sagetrac-mraum
Copy link
Mannequin

sagetrac-mraum mannequin commented Mar 17, 2011

comment:33

Well, yes. We really need to run the test on a setup without Magma then. I add the fix for this problem (which is review2 patch rebased to current Sage).

@sagetrac-mraum

This comment has been minimized.

@sagetrac-mraum
Copy link
Mannequin

sagetrac-mraum mannequin commented Mar 17, 2011

Attachment: trac-7870-magma-doctest-review4.patch.gz

@JohnCremona
Copy link
Member

comment:34

With the earlier patch (patch3) and the latest magma I tested everything and got no errors, so it seems like a waste of (my) time to do it all again.

I do not understand mraum's comments about doing tests on a machine with no magma. I ran complete tests with and without -only-optional=magma.

@jdemeyer
Copy link

Merged: sage-4.7.alpha2

@jdemeyer
Copy link

comment:35

Works now without magma.

@jdemeyer
Copy link

Changed reviewer from Martin Raum, John Cremona to Martin Raum, John Cremona, Jeroen Demeyer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants