Opened 2 years ago
Closed 21 months ago
#24544 closed enhancement (fixed)
cleaning linbox declarations + reimplement modn interface
Reported by:  vdelecroix  Owned by:  

Priority:  major  Milestone:  sage8.4 
Component:  interfaces  Keywords:  linbox 
Cc:  cpernet, tscrim  Merged in:  
Authors:  Vincent Delecroix  Reviewers:  Travis Scrimshaw, Jeroen Demeyer 
Report Upstream:  N/A  Work issues:  
Branch:  8acfbc9 (Commits)  Commit:  8acfbc98500f2ef619c7df6f2e9800804ea61e75 
Dependencies:  Stopgaps: 
Description (last modified by )
We organize linbox declarations (sage/libs/linbox/
) and make the Sage code almost independent of the interfaces/sage
that is in linbox (see #26178 for the last step).
Switching from LinBox::DenseVector<Givaro::Modular<unsigned int>>
to LinBox::DenseVector<Givaro::Modular<int64_t>>
presented some compilation issues (see discussion from comment:43). That will be dealt with in #26178.
(see also #22872 for the linbox task ticket)
Some upstream reports to linbox and Cython
 Cython bug 1 (RuntimeError) (not relevant anymore)
 Cython bug 2 (cimported module vs typedef in classes) (not relevant anymore)
 LinBox issue (solve is broken for rectangular matrices)
 LinBox issue 117 (givaro modular field using
uint64_t
)
Attachments (1)
Change History (66)
comment:1 Changed 2 years ago by
 Branch set to u/vdelecroix/24544experimental
 Commit set to 2a92e3db7728fe2eea0dfdae53b82927e5bad573
 Summary changed from cleaning linbox declarations to cleaning linbox declarations + reimplement modn interface
comment:2 Changed 2 years ago by
 Description modified (diff)
comment:3 Changed 2 years ago by
 Description modified (diff)
comment:4 Changed 2 years ago by
 Commit changed from 2a92e3db7728fe2eea0dfdae53b82927e5bad573 to eaef554ddedec1f411a0cdd8d2be8543618e7423
Branch pushed to git repo; I updated commit sha1. New commits:
eaef554  mention thread about Cython problems

comment:5 Changed 2 years ago by
 Description modified (diff)
 Report Upstream changed from N/A to Reported upstream. No feedback yet.
comment:6 Changed 2 years ago by
 Description modified (diff)
comment:7 Changed 2 years ago by
 Commit changed from eaef554ddedec1f411a0cdd8d2be8543618e7423 to 7ec96109cfd54cf129d934139a5a0f006b8708dd
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
7f21155  24544: cleaning linbox headers

f89020a  24544: fix matrix_modn_dense

827f214  24544: linbox solvers

74ddca5  24544: fix matrix_modn_sparse

3fb2e57  24544: fix documentation

a395c7b  24544: mention thread about Cython problems

7ec9610  24544: do not compile the sage extension support anymore

comment:8 Changed 2 years ago by
The current branch at 7ec9610
does compile but Sage crashes when launched with
/opt/sage/local/lib/python2.7/sitepackages/sage/matrix/linbox_solver_modn_sparse.pxd in init sage.matrix.matrix_modn_sparse() 6 from sage.modules.vector_modn_dense cimport Vector_modn_dense 7 > 8 cdef class LinBoxSolver_modn_sparse(object): global cdef = undefined global LinBoxSolver_modn_sparse = undefined global object = undefined 9 r""" 10 Solving linear equations in Sage using LinBox. 11 """ 12 cdef object Vin ImportError: /opt/sage/local/lib/python2.7/sitepackages/sage/matrix/linbox_solver_modn_sparse.so: undefined symbol: _ZNK6LinBox15MVProductDomainIN6Givaro7ModularImmEEE22mulColDenseSpecializedINS_10BlasVectorIS3_St6vectorImSaImEEEENS_15TransposeMatrixINS_9Protected19SparseMatrixGenericIS3_S7_ISt4pairImmESaISF_EENS_16VectorCategories23SparseSequenceVectorTagEEENS_16MatrixCategories12RowMatrixTagEEES9_EERT_RKNS_12VectorDomainIS3_EESP_RKT0_RKT1_SJ_
comment:9 Changed 2 years ago by
Why are you using comments like
# return a new LinBox vector from a Sage vector # (such vector has to be deallocated with a "del" statement) # INPUT: # F  LinBox field # v  Sage vector
instead of docstring syntax like
""" Return a new LinBox vector from a Sage vector (such vector has to be deallocated with a "del" statement) INPUT:  F  LinBox field  v  Sage vector """
comment:10 Changed 2 years ago by
Regarding
# without this function, Cython produces a wrong C file # Namely, at compilation # # def _dummy(): raise RuntimeError("this is a cython bug")
could you report that bug on the Cython github page and put a link to that bug report in the comment?
comment:11 Changed 2 years ago by
Same for the other places where you claim "Cython bug".
comment:12 Changed 2 years ago by
This ticket does a lot of things... would it be possible to separate the changes to src/sage/libs/linbox
?
comment:13 Changed 2 years ago by
src/sage/libs/linbox/linbox_flint_interface.pxd
defines only Cython functions, why did you add the # distutils
stuff?
comment:14 Changed 2 years ago by
Typo: Inteer
> Integer
comment:15 followup: ↓ 17 Changed 2 years ago by
Why the explicit PyInt_FromLong
here?
return PyInt_FromLong(A_rank)
Cython knows how to convert a long
to a Python object.
comment:16 Changed 2 years ago by
 Commit changed from 7ec96109cfd54cf129d934139a5a0f006b8708dd to 6e8aa6196a98aa564f9929fb561aca9489368a24
Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
c00f453  24544: fix matrix_modn_sparse

3f364c7  24544: fix documentation

b628180  24544: mention thread about Cython problems

421b7a3  24544: do not compile the sage extension support anymore

85af47a  24544: remove unused import

a13a081  24544: use docstring instead of comments

496e5db  24544: Inteeer > Integer

6e19a83  24544: simplify error management

35b85ec  24544: more simplification in fflas declarations

6e8aa61  no more cython bug

comment:17 in reply to: ↑ 15 Changed 2 years ago by
Replying to jdemeyer:
Why the explicit
PyInt_FromLong
here?return PyInt_FromLong(A_rank)Cython knows how to convert a
long
to a Python object.
Otherwise, I obtain a Python long by default which is not the same behavior as the other rank functions on matrices.
comment:18 Changed 2 years ago by
The current branch based on sage7.2.beta7 (commit 6e8aa61
) crashes similarly to what is described in comment:8.
comment:19 Changed 2 years ago by
 Description modified (diff)
comment:20 Changed 2 years ago by
 Cc tscrim added
comment:21 Changed 2 years ago by
 Branch changed from u/vdelecroix/24544experimental to u/vdelecroix/24544
 Commit changed from 6e8aa6196a98aa564f9929fb561aca9489368a24 to b516d4589db18337f912a5b7c78c07bd8e8ae418
 Milestone changed from sage8.2 to sage8.3
 Status changed from new to needs_review
Apparently, I can not use Modular<uint64_t>
within solve (see LinBox issue 117). The rest looks fine! Needs review :)
New commits:
eb27f6d  24544: cleaning linbox headers

21dcc5c  24544: fix matrix_modn_dense

2f6ec4c  24544: fix matrix_modn_sparse

05fbbb7  24544: fix documentation

b516d45  24544: do not compile the sage extension support anymore

comment:22 Changed 2 years ago by
 Description modified (diff)
comment:23 Changed 2 years ago by
 Commit changed from b516d4589db18337f912a5b7c78c07bd8e8ae418 to 956610a83e4d5f091b6e91dfaccae3772a9f3178
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
eb54e4b  24544: cleaning linbox headers

9d787d3  24544: fix matrix_modn_dense

99ed944  24544: fix matrix_modn_sparse

13f4de4  24544: fix documentation

a1e4933  24544: do not compile the sage extension support anymore

956610a  24544: remove linbox GFq from benchmarks

comment:24 Changed 2 years ago by
 Description modified (diff)
comment:25 followup: ↓ 26 Changed 2 years ago by
From my lookover, everything seems okay, but I don't have as experienced of an eye as Jeroen for these things. So Jeoren, I would appreciate it if you could give it another quick lookover.
Minor nitpick from me is the formatting of the docstrings in conversion.pxd
.
comment:26 in reply to: ↑ 25 ; followup: ↓ 27 Changed 2 years ago by
Replying to tscrim:
From my lookover, everything seems okay, but I don't have as experienced of an eye as Jeroen for these things. So Jeoren, I would appreciate it if you could give it another quick lookover.
Minor nitpick from me is the formatting of the docstrings in
conversion.pxd
.
These are inline C functions that will not appear anywhere and are not even accessible from ??
. These docstrings as for people using the code. Adding extra ``a``
make them less readable I think.
comment:27 in reply to: ↑ 26 Changed 2 years ago by
Replying to vdelecroix:
Replying to tscrim:
Minor nitpick from me is the formatting of the docstrings in
conversion.pxd
.These are inline C functions that will not appear anywhere and are not even accessible from
??
. These docstrings as for people using the code. Adding extra``a``
make them less readable I think.
I don't care that strongly. I am so used to seeing the Sageformatted docstrings, it took me a little bit of extra time to mentally parse it. So if you are opposed to such a change, then I am okay to call it bikeshedding and move on.
comment:28 Changed 2 years ago by
 Commit changed from 956610a83e4d5f091b6e91dfaccae3772a9f3178 to 7449b401b31ab17886fd6591efa79ee466510dd9
comment:29 followup: ↓ 30 Changed 2 years ago by
What does this mean?
NOTE: this function is not part of the interface (ie the .pxd file) to keep the module Ccompatible
comment:30 in reply to: ↑ 29 Changed 2 years ago by
Replying to jdemeyer:
What does this mean?
NOTE: this function is not part of the interface (ie the .pxd file) to keep the module Ccompatible
The pxd file only contains C declarations and is hence compatible with Cython C files compiled with gcc
not g++
. I don't know whether it makes a difference but as flint is a pure C library I thought it would be better this way.
comment:31 Changed 2 years ago by
 Commit changed from 7449b401b31ab17886fd6591efa79ee466510dd9 to d63c2937e7ad6aa8a9d7c266cc209075982a275f
Branch pushed to git repo; I updated commit sha1. New commits:
d63c293  24544: rearrange imports + remove PyInt_From_Long

comment:32 Changed 2 years ago by
Still conflicts with #24742.
comment:33 Changed 2 years ago by
The determinant of a 0x0 matrix should be 1.
comment:34 Changed 2 years ago by
 Commit changed from d63c2937e7ad6aa8a9d7c266cc209075982a275f to 100f5d3598ce99258fa5c47a572b1cece10cabae
comment:35 Changed 2 years ago by
 Commit changed from 100f5d3598ce99258fa5c47a572b1cece10cabae to 7abb3a3e701b711d68500d727cc282bf2cdd3e7f
Branch pushed to git repo; I updated commit sha1. New commits:
7abb3a3  24544: simplify imports to avoid conflicts with 24742

comment:36 Changed 2 years ago by
 Commit changed from 7abb3a3e701b711d68500d727cc282bf2cdd3e7f to ae94b81c1d160d00675206c86289f12968b403fe
Branch pushed to git repo; I updated commit sha1. New commits:
ae94b81  24544: remove a removed line

comment:37 Changed 2 years ago by
Anything more? I would like this one merged before touching #23214.
comment:38 Changed 2 years ago by
 Reviewers set to Travis Scrimshaw, Jeroen Demeyer
 Status changed from needs_review to positive_review
LGTM.
comment:40 Changed 2 years ago by
 Commit changed from ae94b81c1d160d00675206c86289f12968b403fe to 51012d0a6c7527cbd8e6114207f7f762f39d3710
Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
c979d74  24544: fix matrix_modn_dense

164c9a6  24544: fix matrix_modn_sparse

8202e42  24544: fix documentation

a4cf45f  24544: do not compile the sage extension support anymore

d2ce10a  24544: remove linbox GFq from benchmarks

c14f5e9  24544: simpler creation of Vector_modn_dense

e6a283a  24544: remove backslashes in fflas.pxd

ccbb126  24544: rearrange imports + remove PyInt_From_Long

f10cd77  24544: better phrasing

51012d0  24544: cleaning det/rank for sparse modn matrices

comment:41 Changed 2 years ago by
 Status changed from needs_work to needs_review
rebased on 8.3beta2 (build and all tests pass in matrix/ currently building doc)
comment:42 Changed 2 years ago by
 Status changed from needs_review to positive_review
Everything's fine (as far as I can test). Since nothing changed (beyond the rebase) I am setting back to positive review.
comment:43 Changed 2 years ago by
 Status changed from positive_review to needs_work
Fails on OSX:
[sagelib8.3.beta2] In file included from build/cythonized/sage/matrix/matrix_modn_sparse.cpp:689: [sagelib8.3.beta2] In file included from /Users/buildslavesage/slave/sage_git/build/local/include/linbox/matrix/densematrix.h:79: [sagelib8.3.beta2] In file included from /Users/buildslavesage/slave/sage_git/build/local/include/linbox/matrix/densematrix/blasmatrix.h:44: [sagelib8.3.beta2] /Users/buildslavesage/slave/sage_git/build/local/include/linbox/vector/blasvector.h:306:11: error: member reference base type 'const long' is not a structure or union [sagelib8.3.beta2] _size(V.size()),_1stride(1),_rep(V.size(), F.zero),_ptr(&_rep[0]),_field(&F) [sagelib8.3.beta2] ~^~~~~
Full log: http://build.sagemath.org/#/builders/32/builds/83/steps/5/logs/stdio
comment:44 Changed 2 years ago by
 Keywords linbox added
comment:45 followup: ↓ 46 Changed 2 years ago by
What's the status of the problem on OsX ?
comment:46 in reply to: ↑ 45 Changed 2 years ago by
Replying to chapoton:
What's the status of the problem on OsX ?
Thanks for your concern. I don't have any OSX machine and don't plan to buy any. I can not help with this issue. The only thing I know is comment:43.
comment:47 followup: ↓ 48 Changed 2 years ago by
is this not an issue that should be reported to linbox ?
comment:48 in reply to: ↑ 47 Changed 2 years ago by
Replying to chapoton:
is this not an issue that should be reported to linbox ?
Would be better to have something reproducible outside of Sage. Otherwise it might just be closed by upstream as was issue 118.
comment:49 followup: ↓ 50 Changed 2 years ago by
but this looks like something that breaks the build, nothing really sage related ?
comment:50 in reply to: ↑ 49 Changed 2 years ago by
Replying to chapoton:
but this looks like something that breaks the build, nothing really sage related ?
Sure, you can open an issue on github. My comment was just what I thought was best.
comment:51 Changed 23 months ago by
The link given by Volker in comment:43 leads to an empty page.
Is there somebody around with a mac, that could help ? Or maybe Clement has an idea on the problem ?
comment:52 Changed 23 months ago by
I tried this on my laptop running OS X with git trac try 24544
and make
and it failed, I have attached the log. If anyone has suggestions I'm happy to try them out on my machine.
comment:53 Changed 23 months ago by
according to the log given by Alex (thanks !), and by looking at matrix_modn_sparse.cpp
, maybe the problematic lines are
"sage/matrix/matrix_modn_sparse.pyx":1053
and
"sage/matrix/matrix_modn_sparse.pyx":1146
Namely
cdef DenseVector_Modular_int64 * b = new DenseVector_Modular_int64(F[0], self._nrows)
and
cdef DenseVector_Modular_int64 * b = new DenseVector_Modular_int64(F[0], self._nrows)
comment:54 Changed 22 months ago by
 Milestone changed from sage8.3 to sage8.4
update milestone 8.3 > 8.4
comment:55 Changed 21 months ago by
This is why I don't understand. The exact same lines appear in the function new_linbox_vector_modn_dense
(sage/libs/linbox/conversion.pxd
) and correspond to the constructor DenseVector_Modular_int64 (Field &F, long& m)
of the class DenseVector_Modular_int64
that corresponds to LinBox::DenseVector<Givaro::Modular<int64_t>>
.
comment:56 Changed 21 months ago by
I would really like to see progress here, and on the sequel ticket.. but not able to help..
Branch is red now.
comment:57 Changed 21 months ago by
I am working on it. I will isolate the problematic change from LinBox::DenseVector<Givaro::Modular<unsigned int>>
to LinBox::DenseVector<Givaro::Modular<int64_t>>
. Clement gave me the advice to do this change but since then (~6 months ago) it does not compile and Clement did not answer any of my mail.
comment:58 Changed 21 months ago by
 Commit changed from 51012d0a6c7527cbd8e6114207f7f762f39d3710 to 8acfbc98500f2ef619c7df6f2e9800804ea61e75
comment:59 Changed 21 months ago by
 Description modified (diff)
comment:60 Changed 21 months ago by
 Description modified (diff)
 Status changed from needs_work to needs_review
comment:61 Changed 21 months ago by
 Report Upstream changed from Reported upstream. No feedback yet. to N/A
comment:62 Changed 21 months ago by
Strange doctests failure with the petitbonum patchbot
sage t long warnlong 58.5 src/sage/interfaces/ecm.py ********************************************************************** File "src/sage/interfaces/ecm.py", line 457, in sage.interfaces.ecm.ECM.one_curve Failed example: f.one_curve(n, B1=500000, algorithm="P1") Expected: [67872792749091946529, 6366805760909027985741435139224233] Got: [1, 432132887883903108009802143314445113500016816977037257] ********************************************************************** File "src/sage/interfaces/ecm.py", line 460, in sage.interfaces.ecm.ECM.one_curve Failed example: f.one_curve(n, B1=2000, algorithm="P+1", x0=5) Expected: [328006342451, 6366805760909027985741435139224233] Got: [1, 2088352670731726262548647919416588631875815083] ********************************************************************** 1 item had failures: 2 of 9 in sage.interfaces.ecm.ECM.one_curve [44 tests, 2 failures, 5.94 s]
comment:63 Changed 21 months ago by
Though patchbot arando is happy :)
comment:64 Changed 21 months ago by
 Status changed from needs_review to positive_review
LGTM (from the description, the failures in comment:43 should no longer apply). (I've also seen the failures in comment:62 on other tickets that patchbot has tested.)
comment:65 Changed 21 months ago by
 Branch changed from u/vdelecroix/24544 to 8acfbc98500f2ef619c7df6f2e9800804ea61e75
 Resolution set to fixed
 Status changed from positive_review to closed
See the attached branch for an experimental version. All tests pass on my computer. Though many tests in
sage/matrix/linbox_solver_modn_sparse.pyx
are there for showing potential bugs in linboxsolve
.New commits:
cleaning linbox headers
fix matrix_modn_dense
linbox solvers
fix matrix_modn_sparse
fix documentation