Opened 3 years ago
Last modified 2 years ago
#21327 needs_work enhancement
fix/improve linbox binding
Reported by:  Bouillaguet  Owned by:  

Priority:  major  Milestone:  sage7.4 
Component:  interfaces  Keywords:  sd75 
Cc:  cpernet  Merged in:  
Authors:  Charles Bouillaguet  Reviewers:  
Report Upstream:  Reported upstream. No feedback yet.  Work issues:  
Branch:  u/Bouillaguet/linbox_cpp (Commits)  Commit:  6fbe826126cc4442208bf9e688acd14de3d54994 
Dependencies:  #21321,#21341  Stopgaps: 
Description (last modified by )
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 Conly 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: https://github.com/linboxteam/linbox/issues/35
Change History (28)
comment:1 Changed 3 years ago by
 Branch set to u/Bouillaguet/linbox_cpp
comment:2 Changed 3 years ago by
 Commit set to 774253f17313501205ca8b8aa767d6a12b235575
 Keywords sd75 added
comment:3 Changed 3 years ago by
 Cc cpernet added
comment:4 Changed 3 years ago by
 Branch u/Bouillaguet/linbox_cpp deleted
 Commit 774253f17313501205ca8b8aa767d6a12b235575 deleted
comment:5 Changed 3 years ago by
 Branch set to u/Bouillaguet/linbox_cpp
comment:6 Changed 3 years ago by
 Commit set to 29b6d1efedf2ebf9beaaae0ddb63b59d6e4b4197
comment:7 Changed 3 years ago by
 Dependencies changed from #21321 to #21321,#21341
comment:8 Changed 3 years ago by
 Commit changed from 29b6d1efedf2ebf9beaaae0ddb63b59d6e4b4197 to 3a77514e1993b45d03df390e44c28350b3490652
Branch pushed to git repo; I updated commit sha1. New commits:
3a77514  Improve LinBox C++ bindings

comment:9 Changed 3 years ago by
 Status changed from new to needs_review
comment:10 Changed 3 years ago by
Just a quick comment for now: this branch contains a lot of commentedout 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.
comment:11 Changed 3 years ago by
 Status changed from needs_review to needs_work
OK, Giavro/linbox/fflas have been updated in an incompatible way.
comment:12 Changed 3 years ago by
 Commit changed from 3a77514e1993b45d03df390e44c28350b3490652 to 37ca132c3983ed31e9f601426382dd0b21f7b284
comment:13 Changed 3 years ago by
 Status changed from needs_work to needs_review
Rebased onto new versions of Givaro/Linbox?/etc. Ready for review.
comment:14 Changed 3 years ago by
 Commit changed from 37ca132c3983ed31e9f601426382dd0b21f7b284 to 549e9e86d14d9f73cf0375be65f80f1191fa52c9
Branch pushed to git repo; I updated commit sha1. New commits:
549e9e8  got rid of Givaro's horrible SpyInteger workaround

comment:15 Changed 3 years ago by
Please remove silly commentedout code like
# ``this`` should replace: #from libcpp.vector cimport vector, list # ``that``
(I have no idea what you are trying to say here)
comment:16 Changed 3 years ago by
 Commit changed from 549e9e86d14d9f73cf0375be65f80f1191fa52c9 to 709ef92f8bcd8f409d7c46e03858d61fdc6b3671
Branch pushed to git repo; I updated commit sha1. New commits:
709ef92  remove useless comment

comment:17 Changed 3 years ago by
 Trailing comma:
SparseElimination,
 Don't put Python code in
sig_on()
. This is wrong:sig_on() if typ == 'minpoly': [...] sig_off()
 Instead of using
<SparseMatrixGFp *> self._M
everywhere, why not declare_M
asSparseMatrixGFp*
(or even justSparseMatrixGFp
if that works to avoid the explicitdel
)?
comment:18 followup: ↓ 19 Changed 3 years ago by
 The problem is that because of the LinBox? <> IML incompatibility, the various
*.h
files from linbox must not be pulled intomatrix_integer_dense.pyx
. Thus, they must not appear inlinbox.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 inlinbox.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
.
comment:19 in reply to: ↑ 18 Changed 3 years ago by
 Status changed from needs_review to needs_work
Replying to Bouillaguet:
 The problem is that because of the LinBox? <> IML incompatibility, the various
*.h
files from linbox must not be pulled intomatrix_integer_dense.pyx
. Thus, they must not appear inlinbox.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 inlinbox.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.
comment:20 followup: ↓ 22 Changed 3 years ago by
 Description modified (diff)
 Report Upstream changed from N/A to Reported upstream. No feedback yet.
comment:21 Changed 3 years ago by
 Commit changed from 709ef92f8bcd8f409d7c46e03858d61fdc6b3671 to 6fbe826126cc4442208bf9e688acd14de3d54994
Branch pushed to git repo; I updated commit sha1. New commits:
6fbe826  reviewers comments

comment:22 in reply to: ↑ 20 ; followup: ↓ 23 Changed 3 years ago by
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:
 Fixing upstream
 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 backwardincompatible 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.
comment:23 in reply to: ↑ 22 Changed 3 years ago by
Replying to 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:
 Fixing upstream
 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 https://github.com/linboxteam/linbox/issues/35 I proposed some solutions.
comment:24 Changed 3 years ago by
OK, I will patch it.
comment:25 Changed 3 years ago by
I applied Jeroen's patch upstream.
comment:26 Changed 3 years ago by
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?
comment:27 Changed 3 years ago by
Oops, my bad, I should have remembered this ticket when Vincent told me about his work on #22872.
comment:28 Changed 2 years ago by
Ticket #24544 will finish moving the Sage interface to LinBox inside Sage (commit 7ec9610 removes the enablesage
option). I am only facing a strange missing symbol issue, see #24554comment:8.
Last 10 new commits:
fixed vector_integer_sparse
convert modules/vector_modn_sparse.pxi
Merge branch 'modules_pxi_must_die_hard' into develop
fixed vector_modn_sparse
added some C++ bindings to linbox interface
linbox: added declaration of Modular<int> field
linboxsage interface: notes for later
linboxsage interface: declaring more C++ linbox operations
more Linbox declarations
trying to generate a LinBox matrix inside Sage