Opened 17 months ago

Closed 9 months ago

#24544 closed enhancement (fixed)

cleaning linbox declarations + reimplement modn interface

Reported by: vdelecroix Owned by:
Priority: major Milestone: sage-8.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 vdelecroix)

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

Attachments (1)

log (121.4 KB) - added by alexjbest 11 months ago.
Log of failure

Download all attachments as: .zip

Change History (66)

comment:1 Changed 17 months ago by vdelecroix

  • Branch set to u/vdelecroix/24544-experimental
  • Commit set to 2a92e3db7728fe2eea0dfdae53b82927e5bad573
  • Summary changed from cleaning linbox declarations to cleaning linbox declarations + reimplement modn interface

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 linbox solve.


New commits:

7528ff9cleaning linbox headers
c58ef8bfix matrix_modn_dense
1ad93eflinbox solvers
3813129fix matrix_modn_sparse
2a92e3dfix documentation

comment:2 Changed 17 months ago by vdelecroix

  • Description modified (diff)

comment:3 Changed 17 months ago by vdelecroix

  • Description modified (diff)

comment:4 Changed 17 months ago by git

  • Commit changed from 2a92e3db7728fe2eea0dfdae53b82927e5bad573 to eaef554ddedec1f411a0cdd8d2be8543618e7423

Branch pushed to git repo; I updated commit sha1. New commits:

eaef554mention thread about Cython problems

comment:5 Changed 17 months ago by vdelecroix

  • Description modified (diff)
  • Report Upstream changed from N/A to Reported upstream. No feedback yet.

comment:6 Changed 17 months ago by vdelecroix

  • Description modified (diff)

comment:7 Changed 16 months ago by git

  • Commit changed from eaef554ddedec1f411a0cdd8d2be8543618e7423 to 7ec96109cfd54cf129d934139a5a0f006b8708dd

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

7f2115524544: cleaning linbox headers
f89020a24544: fix matrix_modn_dense
827f21424544: linbox solvers
74ddca524544: fix matrix_modn_sparse
3fb2e5724544: fix documentation
a395c7b24544: mention thread about Cython problems
7ec961024544: do not compile the sage extension support anymore

comment:8 Changed 16 months ago by vdelecroix

The current branch at 7ec9610 does compile but Sage crashes when launched with

/opt/sage/local/lib/python2.7/site-packages/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/site-packages/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 16 months ago by jdemeyer

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 16 months ago by jdemeyer

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 16 months ago by jdemeyer

Same for the other places where you claim "Cython bug".

Last edited 16 months ago by jdemeyer (previous) (diff)

comment:12 Changed 16 months ago by jdemeyer

This ticket does a lot of things... would it be possible to separate the changes to src/sage/libs/linbox?

comment:13 Changed 16 months ago by jdemeyer

src/sage/libs/linbox/linbox_flint_interface.pxd defines only Cython functions, why did you add the # distutils stuff?

comment:14 Changed 16 months ago by jdemeyer

Typo: Inteer -> Integer

comment:15 follow-up: Changed 16 months ago by jdemeyer

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 15 months ago by git

  • Commit changed from 7ec96109cfd54cf129d934139a5a0f006b8708dd to 6e8aa6196a98aa564f9929fb561aca9489368a24

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:

c00f45324544: fix matrix_modn_sparse
3f364c724544: fix documentation
b62818024544: mention thread about Cython problems
421b7a324544: do not compile the sage extension support anymore
85af47a24544: remove unused import
a13a08124544: use docstring instead of comments
496e5db24544: Inteeer -> Integer
6e19a8324544: simplify error management
35b85ec24544: more simplification in fflas declarations
6e8aa61no more cython bug

comment:17 in reply to: ↑ 15 Changed 15 months ago by vdelecroix

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 15 months ago by vdelecroix

The current branch based on sage-7.2.beta7 (commit 6e8aa61) crashes similarly to what is described in comment:8.

comment:19 Changed 13 months ago by vdelecroix

  • Description modified (diff)

comment:20 Changed 13 months ago by tscrim

  • Cc tscrim added

comment:21 Changed 13 months ago by vdelecroix

  • Branch changed from u/vdelecroix/24544-experimental to u/vdelecroix/24544
  • Commit changed from 6e8aa6196a98aa564f9929fb561aca9489368a24 to b516d4589db18337f912a5b7c78c07bd8e8ae418
  • Milestone changed from sage-8.2 to sage-8.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:

eb27f6d24544: cleaning linbox headers
21dcc5c24544: fix matrix_modn_dense
2f6ec4c24544: fix matrix_modn_sparse
05fbbb724544: fix documentation
b516d4524544: do not compile the sage extension support anymore

comment:22 Changed 13 months ago by vdelecroix

  • Description modified (diff)

comment:23 Changed 13 months ago by git

  • Commit changed from b516d4589db18337f912a5b7c78c07bd8e8ae418 to 956610a83e4d5f091b6e91dfaccae3772a9f3178

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

eb54e4b24544: cleaning linbox headers
9d787d324544: fix matrix_modn_dense
99ed94424544: fix matrix_modn_sparse
13f4de424544: fix documentation
a1e493324544: do not compile the sage extension support anymore
956610a24544: remove linbox GFq from benchmarks

comment:24 Changed 13 months ago by vdelecroix

  • Description modified (diff)

comment:25 follow-up: Changed 13 months ago by tscrim

From my look-over, 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 look-over.

Minor nitpick from me is the formatting of the docstrings in conversion.pxd.

comment:26 in reply to: ↑ 25 ; follow-up: Changed 13 months ago by vdelecroix

Replying to tscrim:

From my look-over, 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 look-over.

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.

Last edited 13 months ago by vdelecroix (previous) (diff)

comment:27 in reply to: ↑ 26 Changed 13 months ago by tscrim

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 Sage-formatted 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 13 months ago by git

  • Commit changed from 956610a83e4d5f091b6e91dfaccae3772a9f3178 to 7449b401b31ab17886fd6591efa79ee466510dd9

Branch pushed to git repo; I updated commit sha1. New commits:

02e057e24544: simpler creation of Vector_modn_dense
7449b4024544: remove backslashes in fflas.pxd

comment:29 follow-up: Changed 13 months ago by jdemeyer

What does this mean?

NOTE: this function is not part of the interface (ie the .pxd file) to keep the
module C-compatible

comment:30 in reply to: ↑ 29 Changed 13 months ago by vdelecroix

Replying to jdemeyer:

What does this mean?

NOTE: this function is not part of the interface (ie the .pxd file) to keep the
module C-compatible

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 13 months ago by git

  • Commit changed from 7449b401b31ab17886fd6591efa79ee466510dd9 to d63c2937e7ad6aa8a9d7c266cc209075982a275f

Branch pushed to git repo; I updated commit sha1. New commits:

d63c29324544: rearrange imports + remove PyInt_From_Long

comment:32 Changed 13 months ago by jdemeyer

Still conflicts with #24742.

comment:33 Changed 13 months ago by jdemeyer

The determinant of a 0x0 matrix should be 1.

comment:34 Changed 13 months ago by git

  • Commit changed from d63c2937e7ad6aa8a9d7c266cc209075982a275f to 100f5d3598ce99258fa5c47a572b1cece10cabae

Branch pushed to git repo; I updated commit sha1. New commits:

0612cbf24544: better phrasing
100f5d324544: cleaning det/rank for sparse modn matrices

comment:35 Changed 13 months ago by git

  • Commit changed from 100f5d3598ce99258fa5c47a572b1cece10cabae to 7abb3a3e701b711d68500d727cc282bf2cdd3e7f

Branch pushed to git repo; I updated commit sha1. New commits:

7abb3a324544: simplify imports to avoid conflicts with 24742

comment:36 Changed 13 months ago by git

  • Commit changed from 7abb3a3e701b711d68500d727cc282bf2cdd3e7f to ae94b81c1d160d00675206c86289f12968b403fe

Branch pushed to git repo; I updated commit sha1. New commits:

ae94b8124544: remove a removed line

comment:37 Changed 13 months ago by vdelecroix

Anything more? I would like this one merged before touching #23214.

comment:38 Changed 13 months ago by tscrim

  • Reviewers set to Travis Scrimshaw, Jeroen Demeyer
  • Status changed from needs_review to positive_review

LGTM.

comment:39 Changed 13 months ago by vbraun

  • Status changed from positive_review to needs_work

Merge conflict

comment:40 Changed 12 months ago by git

  • Commit changed from ae94b81c1d160d00675206c86289f12968b403fe to 51012d0a6c7527cbd8e6114207f7f762f39d3710

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:

c979d7424544: fix matrix_modn_dense
164c9a624544: fix matrix_modn_sparse
8202e4224544: fix documentation
a4cf45f24544: do not compile the sage extension support anymore
d2ce10a24544: remove linbox GFq from benchmarks
c14f5e924544: simpler creation of Vector_modn_dense
e6a283a24544: remove backslashes in fflas.pxd
ccbb12624544: rearrange imports + remove PyInt_From_Long
f10cd7724544: better phrasing
51012d024544: cleaning det/rank for sparse modn matrices

comment:41 Changed 12 months ago by vdelecroix

  • Status changed from needs_work to needs_review

rebased on 8.3-beta2 (build and all tests pass in matrix/ currently building doc)

comment:42 Changed 12 months ago by vdelecroix

  • 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 12 months ago by vbraun

  • Status changed from positive_review to needs_work

Fails on OSX:

[sagelib-8.3.beta2] In file included from build/cythonized/sage/matrix/matrix_modn_sparse.cpp:689:
[sagelib-8.3.beta2] In file included from /Users/buildslave-sage/slave/sage_git/build/local/include/linbox/matrix/dense-matrix.h:79:
[sagelib-8.3.beta2] In file included from /Users/buildslave-sage/slave/sage_git/build/local/include/linbox/matrix/densematrix/blas-matrix.h:44:
[sagelib-8.3.beta2] /Users/buildslave-sage/slave/sage_git/build/local/include/linbox/vector/blas-vector.h:306:11: error: member reference base type 'const long' is not a structure or union
[sagelib-8.3.beta2]                         _size(V.size()),_1stride(1),_rep(V.size(), F.zero),_ptr(&_rep[0]),_field(&F)
[sagelib-8.3.beta2]                               ~^~~~~

Full log: http://build.sagemath.org/#/builders/32/builds/83/steps/5/logs/stdio

comment:44 Changed 11 months ago by chapoton

  • Keywords linbox added

comment:45 follow-up: Changed 11 months ago by chapoton

What's the status of the problem on OsX ?

comment:46 in reply to: ↑ 45 Changed 11 months ago by vdelecroix

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 follow-up: Changed 11 months ago by chapoton

is this not an issue that should be reported to linbox ?

comment:48 in reply to: ↑ 47 Changed 11 months ago by vdelecroix

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 follow-up: Changed 11 months ago by chapoton

but this looks like something that breaks the build, nothing really sage related ?

comment:50 in reply to: ↑ 49 Changed 11 months ago by vdelecroix

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 11 months ago by chapoton

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 ?

Last edited 11 months ago by chapoton (previous) (diff)

Changed 11 months ago by alexjbest

Log of failure

comment:52 Changed 11 months ago by alexjbest

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 10 months ago by chapoton

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)
Last edited 10 months ago by chapoton (previous) (diff)

comment:54 Changed 10 months ago by vdelecroix

  • Milestone changed from sage-8.3 to sage-8.4

update milestone 8.3 -> 8.4

comment:55 Changed 9 months ago by vdelecroix

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 9 months ago by chapoton

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 9 months ago by vdelecroix

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.

Last edited 9 months ago by vdelecroix (previous) (diff)

comment:58 Changed 9 months ago by git

  • Commit changed from 51012d0a6c7527cbd8e6114207f7f762f39d3710 to 8acfbc98500f2ef619c7df6f2e9800804ea61e75

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

2c157ff24544: cleaning linbox headers
b037c8724544: fix matrix_modn_dense
a12f0a124554: fix matrix modn sparse
52d8a3424544: fix documentation
8acfbc924544: remove linbox GFq from benchmarks

comment:59 Changed 9 months ago by vdelecroix

  • Description modified (diff)

comment:60 Changed 9 months ago by vdelecroix

  • Description modified (diff)
  • Status changed from needs_work to needs_review

comment:61 Changed 9 months ago by vdelecroix

  • Report Upstream changed from Reported upstream. No feedback yet. to N/A

comment:62 Changed 9 months ago by vdelecroix

Strange doctests failure with the petitbonum patchbot

sage -t --long --warn-long 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="P-1")
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 9 months ago by vdelecroix

Though patchbot arando is happy :-)

comment:64 Changed 9 months ago by tscrim

  • 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 9 months ago by vbraun

  • Branch changed from u/vdelecroix/24544 to 8acfbc98500f2ef619c7df6f2e9800804ea61e75
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.