Opened 6 years ago
Last modified 4 years ago
#21327 needs_work enhancement
fix/improve linbox binding
Reported by: | Bouillaguet | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-7.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, GitHub, GitLab) | 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 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: https://github.com/linbox-team/linbox/issues/35
Change History (28)
comment:1 Changed 6 years ago by
- Branch set to u/Bouillaguet/linbox_cpp
comment:2 Changed 6 years ago by
- Commit set to 774253f17313501205ca8b8aa767d6a12b235575
- Keywords sd75 added
comment:3 Changed 6 years ago by
- Cc cpernet added
comment:4 Changed 6 years ago by
- Branch u/Bouillaguet/linbox_cpp deleted
- Commit 774253f17313501205ca8b8aa767d6a12b235575 deleted
comment:5 Changed 6 years ago by
- Branch set to u/Bouillaguet/linbox_cpp
comment:6 Changed 6 years ago by
- Commit set to 29b6d1efedf2ebf9beaaae0ddb63b59d6e4b4197
comment:7 Changed 6 years ago by
- Dependencies changed from #21321 to #21321,#21341
comment:8 Changed 6 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 6 years ago by
- Status changed from new to needs_review
comment:10 Changed 6 years ago by
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.
comment:11 Changed 6 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 6 years ago by
- Commit changed from 3a77514e1993b45d03df390e44c28350b3490652 to 37ca132c3983ed31e9f601426382dd0b21f7b284
comment:13 Changed 6 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 6 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 6 years ago by
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)
comment:16 Changed 6 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 6 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 follow-up: ↓ 19 Changed 6 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 6 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 follow-up: ↓ 22 Changed 6 years ago by
- Description modified (diff)
- Report Upstream changed from N/A to Reported upstream. No feedback yet.
comment:21 Changed 6 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 ; follow-up: ↓ 23 Changed 6 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 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.
comment:23 in reply to: ↑ 22 Changed 6 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/linbox-team/linbox/issues/35 I proposed some solutions.
comment:24 Changed 6 years ago by
OK, I will patch it.
comment:25 Changed 6 years ago by
I applied Jeroen's patch upstream.
comment:26 Changed 5 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 5 years ago by
Oops, my bad, I should have remembered this ticket when Vincent told me about his work on #22872.
comment:28 Changed 4 years ago by
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.
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
linbox-sage interface: notes for later
linbox-sage interface: declaring more C++ linbox operations
more Linbox declarations
trying to generate a LinBox matrix inside Sage