fix/improve linbox binding

[sagetrac-Bouillaguet]

Linbox has a sage interface, exploited by sage's linbox interface. This unpleasant situation (upstream had to be modified to fit inside sage) was necessary: Linbox is a C++ library heavily using templates, and at that time Cython was not capable of handling C++ libraries. Thus a C-only interface had to be built into LinBox.

Cython has improved, and sage's linbox binding should now be capable of accessing LinBox directly, removing the need for a dedicated interface upstream.

There is a problem that IML conflicts with linbox: ​linbox-team/linbox#35

Depends on #21321
Depends on #21341

Upstream: Reported upstream. No feedback yet.

CC: @ClementPernet

Component: interfaces

Keywords: sd75

Author: Charles Bouillaguet

Branch/Commit: u/Bouillaguet/linbox_cpp @ 6fbe826

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

[sagetrac-Bouillaguet]

Branch: u/Bouillaguet/linbox_cpp

[sagetrac-Bouillaguet]

Commit: 774253f

[sagetrac-Bouillaguet]

Last 10 new commits:

ef6eece	fixed vector_integer_sparse
f0c88db	convert modules/vector_modn_sparse.pxi
2d7ee59	Merge branch 'modules_pxi_must_die_hard' into develop
a32caee	fixed vector_modn_sparse
8cc3644	added some C++ bindings to linbox interface
6fb5e3b	linbox: added declaration of Modular field
218ae93	linbox-sage interface: notes for later
b34482a	linbox-sage interface: declaring more C++ linbox operations
1016f80	more Linbox declarations
774253f	trying to generate a LinBox matrix inside Sage

[sagetrac-Bouillaguet]

Changed keywords from none to sd75

[sagetrac-Bouillaguet]

Changed branch from u/Bouillaguet/linbox_cpp to none

[sagetrac-Bouillaguet]

Changed commit from 774253f to none

[sagetrac-Bouillaguet]

Branch: u/Bouillaguet/linbox_cpp

[sagetrac-git]

Commit: 29b6d1e

[sagetrac-git]

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

6addf0c	could declare and call rank
29b6d1e	substituted direct-rank call to the old rank-wrapper one

[sagetrac-Bouillaguet]

Changed dependencies from #21321 to #21321,#21341

[sagetrac-git]

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

3a77514

Improve LinBox C++ bindings

[sagetrac-git]

Changed commit from 29b6d1e to 3a77514

[jdemeyer]

comment:10

Just a quick comment for now: this branch contains a lot of commented-out code that I don't see the point of. Also several TODO's where it is not clear why you cannot just do them. It is OK to have a TODO, but you should comment them better.

[sagetrac-Bouillaguet]

comment:11

OK, Giavro/linbox/fflas have been updated in an incompatible way.

[sagetrac-git]

Changed commit from 3a77514 to 37ca132

[sagetrac-git]

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

b800be0	Improve LinBox C++ bindings
37ca132	Rebased onto Linbox 1.4.2

[sagetrac-Bouillaguet]

comment:13

Rebased onto new versions of Givaro/Linbox/etc. Ready for review.

[sagetrac-git]

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

549e9e8

got rid of Givaro's horrible SpyInteger workaround

[sagetrac-git]

Changed commit from 37ca132 to 549e9e8

[jdemeyer]

comment:15

Please remove silly commented-out code like

# ``this`` should replace:
#from libcpp.vector cimport vector, list

# ``that``

(I have no idea what you are trying to say here)

[sagetrac-git]

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

709ef92

remove useless comment

[sagetrac-git]

Changed commit from 549e9e8 to 709ef92

[jdemeyer]

comment:17

1. Trailing comma: SparseElimination,

2. Don't put Python code in sig_on(). This is wrong:

sig_on()
if typ == 'minpoly':
    [...]
sig_off()

3. Instead of using <SparseMatrixGFp *> self._M everywhere, why not declare _M as SparseMatrixGFp* (or even just SparseMatrixGFp if that works to avoid the explicit del)?

[sagetrac-Bouillaguet]

comment:18

3. The problem is that because of the LinBox <--> IML incompatibility, the various *.h files from linbox must not be pulled into matrix_integer_dense.pyx. Thus, they must not appear in linbox.pxd. This makes it impossible (or at least more contrived) to describe the type of sparse matrices in LinBox.pxd. And because the class has to be defined in linbox.pxd, I did not see any other solution. This is indeed a workaround.

The idea I tried to follow is that linbox.pyx only deals with linbox datastructures. The rest deals with linbox.pyx.

[jdemeyer]

comment:19

Replying to @sagetrac-Bouillaguet:

3. The problem is that because of the LinBox <--> IML incompatibility, the various *.h files from linbox must not be pulled into matrix_integer_dense.pyx. Thus, they must not appear in linbox.pxd. This makes it impossible (or at least more contrived) to describe the type of sparse matrices in LinBox.pxd. And because the class has to be defined in linbox.pxd, I did not see any other solution. This is indeed a workaround.

I would rather like to fix this problem than to workaround it.

[jdemeyer]

Upstream: Reported upstream. No feedback yet.

[sagetrac-git]

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

6fbe826

reviewers comments

[sagetrac-git]

Changed commit from 709ef92 to 6fbe826

[sagetrac-Bouillaguet]

comment:22

Replying to @jdemeyer:

I would rather like to fix this problem than to workaround it.

I understand, however, as you know, fixing the problem requires:
a. Fixing upstream
b. Opening, reviewing and merging a new ticket for the new upstream package

Upstream evolves slowly (the previous version in sage, 1.3.2, was released in 2012...). I'm afraid that this whole process will take a long time, and I'm worried that this ticket will stall for reasons that are beyond my control. I started working on this 3 weeks ago, and backward-incompatible changes were introduced in both the Givaro and Linbox packages, that have been upgraded in sage recently. As such, I had to figure that out when I rebased.

I'd rather see this ticket go through quickly, instead of seeing it rot.

[jdemeyer]

comment:23

Replying to @sagetrac-Bouillaguet:

Replying to @jdemeyer:

I would rather like to fix this problem than to workaround it.

I understand, however, as you know, fixing the problem requires:
a. Fixing upstream
b. Opening, reviewing and merging a new ticket for the new upstream package

Upstream evolves slowly (the previous version in sage, 1.3.2, was released in 2012...). I'm afraid that this whole process will take a long time

For me personally, you don't have to wait for a new official upstream package. You can just add a patch to the Sage package. In linbox-team/linbox#35 I proposed some solutions.

[sagetrac-Bouillaguet]

comment:24

OK, I will patch it.

[ClementPernet]

comment:25

I applied Jeroen's patch upstream.

[videlec]

comment:26

Hello,

I just discovered this ticket while working on #22924 (see also #22872 and cylinbox). My original aim was actually to use linbox for solving sparse integer systems. However, a cleanup was needed before.

I will read what is in the branch u/Bouillaguet/linbox_cpp before going further. Charles, do you plan continuing working on it?

[ClementPernet]

comment:27

Oops, my bad, I should have remembered this ticket when Vincent told me about his work on #22872.

[videlec]

comment:28

Ticket #24544 will finish moving the Sage interface to LinBox inside Sage (commit 7ec9610 removes the --enable-sage option). I am only facing a strange missing symbol issue, see #24554comment:8.