Opened 12 years ago
Closed 11 years ago
#7870 closed defect (fixed)
dozens of failures in magma optional test suite on skynet (eno) with sage-4.3
Reported by: | was | Owned by: | cremona |
---|---|---|---|
Priority: | major | Milestone: | sage-4.7 |
Component: | interfaces | Keywords: | Magma |
Cc: | Merged in: | sage-4.7.alpha2 | |
Authors: | William Stein, John Cremona | Reviewers: | Martin Raum, John Cremona, Jeroen Demeyer |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
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:
Attachments (8)
Change History (43)
Changed 12 years ago by
comment:1 Changed 12 years ago by
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
comment:2 Changed 12 years ago by
- Status changed from new to needs_review
comment:3 Changed 12 years ago by
- Owner changed from was to cremona
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))
comment:4 follow-up: ↓ 5 Changed 12 years ago by
- Status changed from needs_review to needs_work
Full log is at http://www.warwick.ac.uk/staff/J.E.Cremona/magma_test.log .
comment:5 in reply to: ↑ 4 Changed 12 years ago by
- Status changed from needs_work to needs_review
Replying to cremona:
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.
comment:6 Changed 12 years ago by
- Keywords Magma added
- Reviewers set to John Cremona
- Status changed from needs_review to needs_work
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!
comment:7 Changed 12 years ago by
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
comment:8 Changed 12 years ago by
- Status changed from needs_work to needs_review
comment:9 Changed 12 years ago by
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?
comment:10 Changed 12 years ago by
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.
comment:11 Changed 12 years ago by
A shorter log file with only the failing files is at http://www.warwick.ac.uk/staff/J.E.Cremona/magma.out.short
comment:12 follow-up: ↓ 14 Changed 12 years ago by
Oh boy, it looks like sage-4.4 changed (from when I made the patch) and introduced a *bunch* of new issues :-(. Sigh, I'll have to rewrite a bunch of the interface, evidently. Well at least I know now.
comment:13 Changed 12 years ago by
Is there any explanation of why these doc tests need to be revised? I can see
sage/symbolic/expression.pyx
has a pretty trivial change, since Magma has decided to reverse the order of the outputs.
But others seem really odd.
magma(HyperellipticCurve?(f)).IgusaClebschInvariants?() # optional - magma remove: [ 0, -2048/375, -4096/25, -4881645568/84375 ] add: [ -640, -20480, 1310720, 52160364544 ]
Were the first set of Magma results wrong? If so, why was they used as a doctest?
This is probably my lack of mathematical knowledge showing here, but it appears to me Magma has output something completely different in many cases. Is the new output correct? Was the old output incorrect?
Dave
comment:14 in reply to: ↑ 12 Changed 12 years ago by
- Status changed from needs_review to needs_work
Replying to was:
Oh boy, it looks like sage-4.4 changed (from when I made the patch) and introduced a *bunch* of new issues :-(. Sigh, I'll have to rewrite a bunch of the interface, evidently. Well at least I know now.
Based on this, I'm changing this to needs_work. Not volunteering to review, just trying to clean up trac a bit!
comment:15 Changed 11 years ago by
- Reviewers John Cremona deleted
- Status changed from needs_work to needs_review
New patch passes all optional doctests on skynet (eno) with 4.6.1 and Magma 2.17-4.
comment:16 follow-up: ↓ 19 Changed 11 years ago by
- Reviewers set to Martin Raum
- Status changed from needs_review to positive_review
I had to change one doctest, such that it also fits my output. This is definitely no issue, as I just added ... for the time. Apart from this: Perfect!
For the record, the patchbot and the release manager:
Apply trac_7870-magma-doctest.patch Apply trac-7870-magma-doctest-review.patch
Changed 11 years ago by
comment:17 Changed 11 years ago by
I'm happy with the reviewer's patch, and note that this ticket is still marked "positive review".
comment:18 Changed 11 years ago by
trac_7870-magma-doctest.patch and trac-7870-magma-doctest-review.patch when applied to 4.6.2 pass all optional doctests (-only-optional=magma) on skynet/eno with Magma-2.17-5.
comment:19 in reply to: ↑ 16 ; follow-up: ↓ 20 Changed 11 years ago by
- Description modified (diff)
- Reviewers changed from Martin Raum to Martin Raum, John Cremona
Replying to mraum:
For the record, the patchbot and the release manager:
Apply trac_7870-magma-doctest.patch Apply trac-7870-magma-doctest-review.patch
Please in the future write this in the ticket description. Thanks!
comment:20 in reply to: ↑ 19 Changed 11 years ago by
Replying to jdemeyer:
Replying to mraum:
For the record, the patchbot and the release manager:
Apply trac_7870-magma-doctest.patch Apply trac-7870-magma-doctest-review.patch
Please in the future write this in the ticket description. Thanks!
That makes a lot of sence and should probably be put in some sort of guide to using trac. Is there one specifically for Sage?
I know I came unstuck recently when I put a link to an spkg in the description, but that spkg needed tekinfo (or whatever it was), so Francois posted one which did not need it. But mine was in the description, his one was less prominently placed in the notes, so you used mine and found it did not work.
At the very least, specifying the locations of .spkgs and what patches needed to be applied, should be mentioned on sage-devel, but ideally we need it documented on the trac server.
Dave
comment:21 follow-up: ↓ 22 Changed 11 years ago by
- Status changed from positive_review to needs_work
All doctests involving magma should be marked # optional - magma
. I get various failures in sage/rings/number_field/number_field.py
.
comment:22 in reply to: ↑ 21 Changed 11 years ago by
Replying to jdemeyer:
All doctests involving magma should be marked
# optional - magma
. I get various failures insage/rings/number_field/number_field.py
.
Apologies, this refers to the new function I put in (_magma_polynomial_) where I did not tag the doctests as optional. It's my fault (though William was sitting next to me at the time, so I blame him too!)
Changed 11 years ago by
comment:23 Changed 11 years ago by
And it's my fault as well. I'm terribly sorry! John, do you have a maschine without magma to run tests for the above fix on? It was only one function that was missed.
Martin
comment:24 Changed 11 years ago by
I don't think it is necessary to have a machine without magma. Just run a complete test both with and with out the --optional-magma. It is faster to run with that flag since only doctests with the optional magma flag are run.
comment:25 Changed 11 years ago by
- Description modified (diff)
- Status changed from needs_work to needs_review
comment:26 Changed 11 years ago by
This seems right. So, if everything is ok for you, also, give it a positive review.
comment:27 Changed 11 years ago by
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
comment:28 Changed 11 years ago by
- Status changed from needs_review to needs_work
This needs to be rebased to sage-4.7.alpha1.
Changed 11 years ago by
comment:29 follow-up: ↓ 30 Changed 11 years ago by
- Description modified (diff)
- Status changed from needs_work to needs_review
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?
comment:30 in reply to: ↑ 29 Changed 11 years ago by
Replying to 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.)
comment:31 Changed 11 years ago by
- Status changed from needs_review to positive_review
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).
comment:32 Changed 11 years ago by
- Status changed from positive_review to needs_work
It seems that your 'review3' patch is based on an older version of the patches, I get the number_field doctest failures again.
comment:33 Changed 11 years ago by
- Description modified (diff)
- Status changed from needs_work to needs_review
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).
Changed 11 years ago by
comment:34 Changed 11 years ago by
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.
comment:35 Changed 11 years ago by
- Merged in set to sage-4.7.alpha2
- Resolution set to fixed
- Reviewers changed from Martin Raum, John Cremona to Martin Raum, John Cremona, Jeroen Demeyer
- Status changed from needs_review to closed
Works now without magma.
this is the output of running the test suite, showing the actual failures.