Opened 2 years ago

Last modified 10 months 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) Commit: 6fbe826126cc4442208bf9e688acd14de3d54994
Dependencies: #21321,#21341 Stopgaps:

Description (last modified by jdemeyer)

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 2 years ago by Bouillaguet

  • Branch set to u/Bouillaguet/linbox_cpp

comment:2 Changed 2 years ago by Bouillaguet

  • Commit set to 774253f17313501205ca8b8aa767d6a12b235575
  • Keywords sd75 added

Last 10 new commits:

ef6eecefixed vector_integer_sparse
f0c88dbconvert modules/vector_modn_sparse.pxi
2d7ee59Merge branch 'modules_pxi_must_die_hard' into develop
a32caeefixed vector_modn_sparse
8cc3644added some C++ bindings to linbox interface
6fb5e3blinbox: added declaration of Modular<int> field
218ae93linbox-sage interface: notes for later
b34482alinbox-sage interface: declaring more C++ linbox operations
1016f80more Linbox declarations
774253ftrying to generate a LinBox matrix inside Sage

comment:3 Changed 2 years ago by Bouillaguet

  • Cc cpernet added

comment:4 Changed 2 years ago by Bouillaguet

  • Branch u/Bouillaguet/linbox_cpp deleted
  • Commit 774253f17313501205ca8b8aa767d6a12b235575 deleted

comment:5 Changed 2 years ago by Bouillaguet

  • Branch set to u/Bouillaguet/linbox_cpp

comment:6 Changed 2 years ago by git

  • Commit set to 29b6d1efedf2ebf9beaaae0ddb63b59d6e4b4197

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

6addf0ccould declare and call rank
29b6d1esubstituted direct-rank call to the old rank-wrapper one

comment:7 Changed 2 years ago by Bouillaguet

  • Dependencies changed from #21321 to #21321,#21341

comment:8 Changed 2 years ago by git

  • Commit changed from 29b6d1efedf2ebf9beaaae0ddb63b59d6e4b4197 to 3a77514e1993b45d03df390e44c28350b3490652

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

3a77514Improve LinBox C++ bindings

comment:9 Changed 2 years ago by Bouillaguet

  • Status changed from new to needs_review

comment:10 Changed 2 years ago by jdemeyer

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 2 years ago by Bouillaguet

  • Status changed from needs_review to needs_work

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

comment:12 Changed 2 years ago by git

  • Commit changed from 3a77514e1993b45d03df390e44c28350b3490652 to 37ca132c3983ed31e9f601426382dd0b21f7b284

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

b800be0Improve LinBox C++ bindings
37ca132Rebased onto Linbox 1.4.2

comment:13 Changed 2 years ago by Bouillaguet

  • Status changed from needs_work to needs_review

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

comment:14 Changed 2 years ago by git

  • Commit changed from 37ca132c3983ed31e9f601426382dd0b21f7b284 to 549e9e86d14d9f73cf0375be65f80f1191fa52c9

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

549e9e8got rid of Givaro's horrible SpyInteger workaround

comment:15 Changed 2 years ago by jdemeyer

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 2 years ago by git

  • Commit changed from 549e9e86d14d9f73cf0375be65f80f1191fa52c9 to 709ef92f8bcd8f409d7c46e03858d61fdc6b3671

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

709ef92remove useless comment

comment:17 Changed 2 years ago by jdemeyer

  1. Trailing comma: SparseElimination,
  1. Don't put Python code in sig_on(). This is wrong:
    sig_on()
    if typ == 'minpoly':
        [...]
    sig_off()
    
  1. 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)?

comment:18 follow-up: Changed 2 years ago by Bouillaguet

  1. 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.

comment:19 in reply to: ↑ 18 Changed 2 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Replying to Bouillaguet:

  1. 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.

comment:20 follow-up: Changed 2 years ago by jdemeyer

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

comment:21 Changed 2 years ago by git

  • Commit changed from 709ef92f8bcd8f409d7c46e03858d61fdc6b3671 to 6fbe826126cc4442208bf9e688acd14de3d54994

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

6fbe826reviewers comments

comment:22 in reply to: ↑ 20 ; follow-up: Changed 2 years ago by 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:

  1. Fixing upstream
  2. 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 2 years ago by jdemeyer

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:

  1. Fixing upstream
  2. 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 2 years ago by Bouillaguet

OK, I will patch it.

comment:25 Changed 2 years ago by cpernet

I applied Jeroen's patch upstream.

comment:26 Changed 19 months ago by vdelecroix

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 19 months ago by cpernet

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

comment:28 Changed 10 months ago by vdelecroix

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.

Note: See TracTickets for help on using tickets.