Opened 11 years ago
Closed 9 years ago
#9511 closed enhancement (duplicate)
Upgrade Givaro to version 3.7.0
Reported by: | pcpa | Owned by: | cpernet |
---|---|---|---|
Priority: | minor | Milestone: | sage-duplicate/invalid/wontfix |
Component: | finite rings | Keywords: | |
Cc: | zimmerma, jpflori | Merged in: | |
Authors: | Reviewers: | Volker Braun | |
Report Upstream: | Reported upstream. Developers acknowledge bug. | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
Givaro 3.6.x is a requirement for LinBox 1.3.0.
- Install http://sage.math.washington.edu/home/malb/spkgs/givaro-3.7.0.spkg
- Apply trac_9511_givaro_3_7_x.2.patch
See https://bitbucket.org/malb/givaro-spkg for SPKG Repository.
Attachments (3)
Change History (71)
comment:1 Changed 11 years ago by
- Summary changed from Suggestion to upgrade givaro 3.3.1 and 3.3.2 [with patch] to Suggestion to upgrade givaro 3.3.1 or 3.3.2 [with patch]
comment:2 Changed 11 years ago by
comment:3 Changed 10 years ago by
- Description modified (diff)
- Milestone set to sage-4.7.2
- Report Upstream changed from None of the above - read trac for reasoning. to N/A
- Summary changed from Suggestion to upgrade givaro 3.3.1 or 3.3.2 [with patch] to Upgrade givaro to 3.3.x series
comment:4 Changed 10 years ago by
- Description modified (diff)
comment:5 Changed 10 years ago by
- Description modified (diff)
- Status changed from new to needs_review
comment:6 follow-up: ↓ 7 Changed 10 years ago by
Note this upgrade won't work unless LinBox? is upgraded as well.
comment:7 in reply to: ↑ 6 Changed 10 years ago by
comment:8 Changed 10 years ago by
spkg that I missed in the description somehow. linbox in a new ticket?
comment:9 Changed 10 years ago by
- Dependencies set to #11718
Yep, I opened #11718 for that but I just accidentally killed the last three hours of pretty boring & tedious work trying to compile 1.1.7. So it'll take a while until I upload an SPKG there.
comment:10 Changed 10 years ago by
- Status changed from needs_review to needs_work
- Summary changed from Upgrade givaro to 3.3.x series to Upgrade Givaro to 3.3.x series
Status update:
We get errors like this
/sage-4.7.1/local/include/givaro/givzpz16table1.inl:424: error: ‘UINT32_MAX’ was not declared in this scope
on sage.math unless we explicitly pass -D__STDC_LIMIT_MACROS
in spkg-install
. This works but is weird because Givaro does set __STDC_LIMIT_MACROS
before it includes <stdint.h>
. Also, I do not have this problem my local machine (running 64-bit Debian/GNU Linux)
comment:11 Changed 9 years ago by
Just for the record:
There's a Givaro 3.2.13.rc1.p4 spkg with a couple of fixes at #12761 (currently still needing review).
comment:12 Changed 9 years ago by
- Description modified (diff)
comment:13 Changed 9 years ago by
I am feeling ambitious today (Brice from the LinBox? team is in shouting distance) so I decided to give this update business another try. Givaro 3.6.0 is the easy part so here it is.
comment:14 Changed 9 years ago by
- Description modified (diff)
comment:15 Changed 9 years ago by
- Description modified (diff)
comment:16 Changed 9 years ago by
- Description modified (diff)
comment:17 Changed 9 years ago by
- Cc zimmerma added
- Status changed from needs_work to needs_review
Updated so that the number theory doctest in the "French book" is deterministic.
comment:18 Changed 9 years ago by
The permissions of spkg-install
and SPKG.txt
should be fixed to make Jeroen happy ;-)
comment:19 Changed 9 years ago by
Fixed to 755 and 644 respectively.
comment:20 follow-ups: ↓ 21 ↓ 28 Changed 9 years ago by
comment:21 in reply to: ↑ 20 Changed 9 years ago by
Replying to vbraun:
Patch doesn't apply on top of the m4ri update (#12840, #12841).
That's weird, I have it applied here:
$ hg qap [r16883 mq:5/5 sage-linbox.patch - 1:09PM] trac_12840_m4ri_new_version.patch trac_12841_m4rie_new_version.patch trac_9511_givaro_3_6_x.patch matrix_modn_dense_no_linbox.patch sage-linbox.patch
Also,
Set([r for r in R])
should be justSet(R)
or (even better in case there are duplicate elements)sorted(R)
I just copied what Paul Zimmermann suggested as a fix for the book. Paul is CCed, so if he agrees, I'll change the doctest accordingly.
comment:22 follow-up: ↓ 24 Changed 9 years ago by
Where does the line
depends = [SAGE_INC + 'givaro/givconfig.h']),
in your module_list.py
come from? Its not in sage-5.1.beta3.
Also (and perhaps related), #11718 is no longer a dependency for this ticket right?
comment:23 Changed 9 years ago by
- Dependencies changed from #11718 to #12883
comment:24 in reply to: ↑ 22 Changed 9 years ago by
Replying to vbraun:
Where does the line
depends = [SAGE_INC + 'givaro/givconfig.h']),in your
module_list.py
come from? Its not in sage-5.1.beta3.
It's from
http://trac.sagemath.org/sage_trac/attachment/ticket/12761/12761_givaro_depends.patch
which was merged in 5.0.1.rc1.
Also (and perhaps related), #11718 is no longer a dependency for this ticket right?
Right, fixed that.
comment:25 Changed 9 years ago by
- Dependencies changed from #12883 to #12883, #12761
comment:26 Changed 9 years ago by
comment:27 Changed 9 years ago by
Lets leave it at the current state but in general its probably best to avoid cyclic dependencies....
comment:28 in reply to: ↑ 20 Changed 9 years ago by
Replying to vbraun:
Also,
Set([r for r in R])
should be justSet(R)
or (even better in case there are duplicate elements)sorted(R)
I prefer to leave Set([r for r in R])
which is more explicit (in my opinion) and to make
the changes minimal in the book. There are no duplicate elements here (in a finite field).
Paul
comment:29 Changed 9 years ago by
The identity list comprehension doesn't add anything to the understanding here. Why not
Set([ s for s in [r for r in R]))
to make it really really explicit? :-P
If anything, this teaches bad habits. The only valid argument imho is that nobody is going to actually look into the sage.tests directory to learn how to use Sage. So I don't really care here but I would never let this stand in a doctest that is visible in the manual.
comment:30 Changed 9 years ago by
- Summary changed from Upgrade Givaro to 3.3.x series to Upgrade Givaro to version 3.7.0
comment:31 Changed 9 years ago by
I'm getting testing failures on sage-5.1.beta5 (this worked fine on beta3, but the cholesky()
method is new in beta5):
[vbraun@volker-desktop matrix]$ sage -t sage/matrix/matrix2.pyx sage -t "devel/sage-main/sage/matrix/matrix2.pyx" ********************************************************************** File "/home/vbraun/opt/sage-5.1.beta5/devel/sage-main/sage/matrix/matrix2.pyx", line 9813: sage: L Expected: [ 3 0 0] [ 4*a^2 + 1 1 0] [ 3*a + 2 a^2 + 2*a + 3 3] Got: [ 2 0 0] [ a^2 + 4 1 0] [ 2*a + 3 a^2 + 2*a + 3 2] ********************************************************************** 1 items had failures: 1 of 65 in __main__.example_105 ***Test Failed*** 1 failures. For whitespace errors, see the file /home/vbraun/.sage//tmp/matrix2_12014.py [12.8 s]
comment:32 follow-up: ↓ 35 Changed 9 years ago by
Compiles fine, but one failure with the testsuite on mark/skynet (SPARC solaris)
/bin/bash ../libtool --tag=CXX --mode=link g++ -O0 -Wall -g -DNDEBUG -UGIVARO_DEBUG -UDEBUG -I/home/vbraun/opt/mark/sage-5.1.beta3/local//include -fPIC -I"/home/vbraun/opt/mark/sage-5.1.beta3/local/include" -static -o te st-trunc test-trunc.o -L../src -L../src/.libs -lgivaro -L/home/vbraun/opt/mark/sage-5.1.beta3/local//lib -lgmpxx -lgmp -L/home/vbraun/opt/mark/sage-5.1.beta3/local//lib -lgmpxx -lgmp libtool: link: g++ -O0 -Wall -g -DNDEBUG -UGIVARO_DEBUG -UDEBUG -I/home/vbraun/opt/mark/sage-5.1.beta3/local//include -fPIC -I/home/vbraun/opt/mark/sage-5.1.beta3/local/include -o test-trunc test-trunc.o -L../src -L../src/.l ibs /home/vbraun/opt/mark/sage-5.1.beta3/spkg/build/givaro-3.7.0/src/src/.libs/libgivaro.a -L/home/vbraun/opt/mark/sage-5.1.beta3/local//lib /home/vbraun/opt/mark/sage-5.1.beta3/local/lib/libgmpxx.so /home/vbraun/opt/mark/sag e-5.1.beta3/local/lib/libstdc++.so -lm /home/vbraun/opt/mark/sage-5.1.beta3/local/lib/libgmp.so -Wl,-R -Wl,/home/vbraun/opt/mark/sage-5.1.beta3/local/lib -Wl,-R -Wl,/home/vbraun/opt/mark/sage-5.1.beta3/local/lib ld: warning: file /home/vbraun/opt/mark/sage-5.1.beta3/local//lib/libstdc++.so: linked to /home/vbraun/opt/mark/sage-5.1.beta3/local/lib/libstdc++.so: attempted multiple inclusion of file g++ -DHAVE_CONFIG_H -I. -I.. -I.. -I/home/vbraun/opt/mark/sage-5.1.beta3/local//include -I../src/kernel/system -I../src/kernel/memory -I../src/kernel/zpz -I../src/kernel/integer -I../src/kernel -I../src/library/poly1 -I../s rc/kernel/bstruct -I../src/library/tools -O0 -Wall -g -DNDEBUG -UGIVARO_DEBUG -UDEBUG -I/home/vbraun/opt/mark/sage-5.1.beta3/local//include -fPIC -I"/home/vbraun/opt/mark/sage-5.1.beta3/local/include" -c -o test-crt.o test- crt.C
looks good but then further down
PASS: test-trunc /bin/bash: line 5: 12309 Bus Error (core dumped) ${dir}$tst FAIL: test-crt PASS: test-geom PASS: test-integer PASS: test-conversion PASS: test-ratrecon ============================================= 1 of 12 tests failed Please report to Jean-Guillaume.Dumas@imag.fr ============================================= make[3]: *** [check-TESTS] Error 1 make[3]: Leaving directory `/home/vbraun/opt/mark/sage-5.1.beta3/spkg/build/givaro-3.7.0/src/tests' make[2]: *** [check-am] Error 2 make[2]: Leaving directory `/home/vbraun/opt/mark/sage-5.1.beta3/spkg/build/givaro-3.7.0/src/tests' make[1]: *** [check-recursive] Error 1 make[1]: Leaving directory `/home/vbraun/opt/mark/sage-5.1.beta3/spkg/build/givaro-3.7.0/src/tests' make: *** [check-recursive] Error 1 Error while running the Givaro testsuite ... exiting
comment:33 Changed 9 years ago by
- Report Upstream changed from N/A to Reported upstream. No feedback yet.
Changed 9 years ago by
comment:34 Changed 9 years ago by
- Description modified (diff)
I rebased the patch to 5.1.beta5. In particular, this patch removes the line from matrix2.pyx
that producses the offending output
sage -t "devel/sage-main/sage/matrix/matrix2.pyx" ********************************************************************** File "/home/vbraun/opt/sage-5.1.beta5/devel/sage-main/sage/matrix/matrix2.pyx", line 9813: sage: L Expected: [ 3 0 0] [ 4*a^2 + 1 1 0] [ 3*a + 2 a^2 + 2*a + 3 3] Got: [ 2 0 0] [ a^2 + 4 1 0] [ 2*a + 3 a^2 + 2*a + 3 2] **********************************************************************
The Cholesky decomposition is not unique over finite fields and we shouldn't test for the output (which can be random depending on which square-root is chosen) but test for L*LT == A. This is done by the line after the line removed in this patch. Hence, correctness is checked. Note that I discussed this off-list Rob Beezer who is the author of the line removed in this patch.
comment:35 in reply to: ↑ 32 Changed 9 years ago by
Replying to vbraun:
PASS: test-trunc /bin/bash: line 5: 12309 Bus Error (core dumped) ${dir}$tst FAIL: test-crt
Shall we set this to needs work then? What's our policy for stuff like this?
comment:36 Changed 9 years ago by
- Report Upstream changed from Reported upstream. No feedback yet. to Reported upstream. Developers acknowledge bug.
- Status changed from needs_review to needs_work
I talked to Jean-Guillaume and he gave me an experimental patch but it didn't help. Here is the gdb backtrace
-bash-3.00$ /home/vbraun/opt/mark/gdb-7.4.1/gdb/gdb test-crt GNU gdb (GDB) 7.4.1 Copyright (C) 2012 Free Software Foundation, Inc. License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html> This is free software: you are free to change and redistribute it. There is NO WARRANTY, to the extent permitted by law. Type "show copying" and "show warranty" for details. This GDB was configured as "sparc-sun-solaris2.10". For bug reporting instructions, please see: <http://www.gnu.org/software/gdb/bugs/>... Reading symbols from /home/vbraun/opt/mark/sage-5.1.beta3/spkg/build/givaro-3.7.0/src/tests/test-crt...done. (gdb) run Starting program: /home/vbraun/opt/mark/sage-5.1.beta3/spkg/build/givaro-3.7.0/src/tests/test-crt [Thread debugging using libthread_db enabled] [New Thread 1 (LWP 1)] Program received signal SIGSEGV, Segmentation fault. [Switching to Thread 1 (LWP 1)] 0x00029030 in Givaro::GFqDom<long>::GFqDom (this=0x7eaec, F=...) at /home/vbraun/opt/mark/sage-5.1.beta3/local//include/givaro/givgfq.h:100 100 _dcharacteristic = F._dcharacteristic; (gdb) bt #0 0x00029030 in Givaro::GFqDom<long>::GFqDom (this=0x7eaec, F=...) at /home/vbraun/opt/mark/sage-5.1.beta3/local//include/givaro/givgfq.h:100 #1 0x00033c3c in Givaro::GivaroMM<Givaro::GFqDom<long> >::initone (p=0x7eaec, V=...) at /home/vbraun/opt/mark/sage-5.1.beta3/local//include/givaro/givaromm.h:297 #2 0x00031110 in Givaro::GivaroMM<Givaro::GFqDom<long> >::initialize (bloc=0x7eaec, s=15, V=...) at /home/vbraun/opt/mark/sage-5.1.beta3/local//include/givaro/givaromm.h:300 #3 0x000268f8 in Givaro::Array0<Givaro::GFqDom<long> >::build (this=0xffbff890, s=15, t=...) at /home/vbraun/opt/mark/sage-5.1.beta3/local//include/givaro/givarray0.inl:30 #4 0x00021ad0 in Givaro::Array0<Givaro::GFqDom<long> >::Array0 (this=0xffbff890, s=15) at /home/vbraun/opt/mark/sage-5.1.beta3/local//include/givaro/givarray0.inl:39 #5 0x0001bf04 in tmain<Givaro::GFqDom<long> > (argc=1, argv=0xffbffb3c, generator=...) at test-crt.C:49 #6 0x00013d00 in main (argc=1, argv=0xffbffb3c) at test-crt.C:151 (gdb) l 95 { 96 zero = F.zero; 97 one = F.one; 98 mOne = F.mOne; 99 _characteristic = F._characteristic; 100 _dcharacteristic = F._dcharacteristic; 101 _exponent = F._exponent; 102 _irred = F._irred; 103 _q = F._q; 104 _qm1 = F._qm1;
I think we should at least wait a bit more if he can come up with a working patch.
comment:37 Changed 9 years ago by
- Description modified (diff)
- Reviewers set to Volker Braun
- Status changed from needs_work to positive_review
The alignment problem stems from the use of placement new in the Givaro array class. Since we currently do not use that in Sage, there is no reason to delay this ticket further. I've created another ticket (#13164) to deal with the SPARC failure.
Positive review.
comment:38 Changed 9 years ago by
- Milestone changed from sage-5.1 to sage-5.2
comment:39 follow-up: ↓ 43 Changed 9 years ago by
- Status changed from positive_review to needs_work
From the spkg-install:
if [ -f "$SAGE_ROOT"/devel/sage/sage/rings/polynomial/multi_polynomial_ideal_libsingular.pyx ]; then echo "Touching extensions linked against Givaro" touch "$SAGE_ROOT"/devel/sage/sage/rings/finite_field_givaro.pyx touch "$SAGE_ROOT"/devel/sage/sage/libs/linbox/linbox.pyx touch "$SAGE_ROOT"/devel/sage/sage/libs/singular/singular.pyx touch "$SAGE_ROOT"/devel/sage/sage/matrix/matrix_mpolynomial_dense.pyx touch "$SAGE_ROOT"/devel/sage/sage/rings/polynomial/multi_polynomial_libsingular.pyx touch "$SAGE_ROOT"/devel/sage/sage/rings/polynomial/multi_polynomial_ideal_libsingular.pyx fi
So THIS is the reason why sage/rings/finite_field_givaro.pyx
magically reappears once in a while. This file has been renamed a long time ago but whenever you reinstall givaro it is recreated. Our setup stuff does take care of recompiling dependent Cython classes, so this whole section is useless. Can you take it out before we ship it?
comment:40 Changed 9 years ago by
comment:41 Changed 9 years ago by
Your repository isn't public, I think. I get an error when following the link.
comment:43 in reply to: ↑ 39 Changed 9 years ago by
- Status changed from positive_review to needs_work
Replying to vbraun:
So THIS is the reason why
sage/rings/finite_field_givaro.pyx
magically reappears once in a while. This file has been renamed a long time ago but whenever you reinstall givaro it is recreated. Our setup stuff does take care of recompiling dependent Cython classes, so this whole section is useless. Can you take it out before we ship it?
This was supposed to be fixed in #12761 (merged in sage-5.0.1.rc1 and sage-5.1.beta4 and clearly mentioned in the comments and dependencies of this ticket!), so your spkg should be rebased to that.
comment:44 Changed 9 years ago by
I've merged the two SPKGs here:
https://bitbucket.org/malb/givaro-spkg/changeset/9b0324505f18
I've also uploaded a new SPKG (MD5: 7db046ff7fcb737234e41fcb2e15f94f
).
Note that Givaro 3.7.0 is GCC 4.7. compliant (at least it compiles on my machine which has GCC 4.7).
comment:45 Changed 9 years ago by
What about givtablelimits.h.patch
? Arguable we don't care about Cygwin any more but then it seems like it can't hurt either. Opinions?
comment:46 follow-up: ↓ 47 Changed 9 years ago by
TBH, I don't care either way.
comment:47 in reply to: ↑ 46 Changed 9 years ago by
Replying to malb:
TBH, I don't care either way.
Well, if you remove a patch, you should have a reason for it...
comment:48 Changed 9 years ago by
- Status changed from needs_work to positive_review
I'm fine without the Cygwin patch, its rather ugly ihmo.
comment:49 Changed 9 years ago by
- Status changed from positive_review to needs_work
You should keep the
echo >&2 Error
instead of
echo Error
and also the Cygwin patch and the $SAGE_LOCAL
quoting (i.e. don't undo something from givaro-3.2.13 unless you have a reason too).
comment:50 Changed 9 years ago by
- Status changed from needs_work to needs_review
Quote and echo business fixed in:
https://bitbucket.org/malb/givaro-spkg/changeset/d9b1505bb161
As far as I can tell Cygwin fixes are not needed any more:
https://lists.gnu.org/archive/html/bug-gnulib/2010-03/msg00217.html
comment:51 Changed 9 years ago by
- Status changed from needs_review to positive_review
Looks good to me!
comment:52 Changed 9 years ago by
- Dependencies changed from #12883, #12761 to #12840, #12841, to be merged with #12883
- Description modified (diff)
comment:53 Changed 9 years ago by
- Milestone changed from sage-5.2 to sage-pending
comment:54 Changed 9 years ago by
- Milestone changed from sage-pending to sage-5.3
comment:55 Changed 9 years ago by
- Cc jpflori added
comment:56 Changed 9 years ago by
- Status changed from positive_review to needs_work
This needs to be rebased to sage-5.3.beta1 (to be released very soon).
comment:57 Changed 9 years ago by
- Description modified (diff)
- Status changed from needs_work to positive_review
Rebased patch for sage-5.3.beta1. Just a trivial rebase in module_list.py
.
comment:58 Changed 9 years ago by
- Milestone changed from sage-5.3 to sage-5.4
comment:59 Changed 9 years ago by
Volker, could you tag the last hg commit here as well?
comment:60 follow-up: ↓ 62 Changed 9 years ago by
I thought that Jeroen's scripts for Sage releases added those tags automatically, but I'm not sure.
comment:61 Changed 9 years ago by
I haven't made any changes to the spkg yet, so I'm not going to partake in the bikeshedding here. The Sage developer guide contains nothing about mandatory hg tagging. In fact, since we frequently make changes in response to reviews before releasing the spkg, really only the release manager can set the tag.
comment:62 in reply to: ↑ 60 Changed 9 years ago by
Replying to jhpalmieri:
I thought that Jeroen's scripts for Sage releases added those tags automatically
Indeed.
comment:63 Changed 9 years ago by
Superseded by #13164.
comment:64 Changed 9 years ago by
Is the Sage library patch still necessary? If so, maybe it should be moved to #13164 and this ticket should be closed as a duplicate.
comment:65 Changed 9 years ago by
I did not try without it (in fact I did but without the linbox patches as well, by mistake), but with it there are no errors.
comment:66 Changed 9 years ago by
- Dependencies #12840, #12841, to be merged with #12883 deleted
Superseded by #13164, close this ticket as duplicate.
comment:67 Changed 9 years ago by
- Milestone changed from sage-5.4 to sage-duplicate/invalid/wontfix
comment:68 Changed 9 years ago by
- Resolution set to duplicate
- Status changed from positive_review to closed
I'm currently working on the upgrade to givaro-3.3.3rc0 together with linbox-1.1.7, since this later version of linbox required very recent changes in givaro (only in 3.3.3rc0).