Opened 4 years ago

Last modified 2 years ago

#21327 needs_work enhancement

fix/improve linbox binding — at Version 20

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: 709ef92f8bcd8f409d7c46e03858d61fdc6b3671
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 (20)

comment:1 Changed 4 years ago by Bouillaguet

  • Branch set to u/Bouillaguet/linbox_cpp

comment:2 Changed 4 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 4 years ago by Bouillaguet

  • Cc cpernet added

comment:4 Changed 4 years ago by Bouillaguet

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

comment:5 Changed 4 years ago by Bouillaguet

  • Branch set to u/Bouillaguet/linbox_cpp

comment:6 Changed 4 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 4 years ago by Bouillaguet

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

comment:8 Changed 4 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 4 years ago by Bouillaguet

  • Status changed from new to needs_review

comment:10 Changed 4 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 4 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 4 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 4 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 4 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 4 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 4 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 4 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 4 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 4 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 Changed 4 years ago by jdemeyer

  • Description modified (diff)
  • Report Upstream changed from N/A to Reported upstream. No feedback yet.
Note: See TracTickets for help on using tickets.