Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better wrapping of IML #21341

Closed
sagetrac-Bouillaguet mannequin opened this issue Aug 26, 2016 · 24 comments
Closed

Better wrapping of IML #21341

sagetrac-Bouillaguet mannequin opened this issue Aug 26, 2016 · 24 comments

Comments

@sagetrac-Bouillaguet
Copy link
Mannequin

sagetrac-Bouillaguet mannequin commented Aug 26, 2016

The IML package should be better wrapped up, for instance in sage/libs/iml. The functions are hidden in sage/matrix/matrix_integer_dense.pyx.

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

For instance, adding :

cdef extern from "linbox/solutions/solve.h" namespace "LinBox":
     pass

on top of sage/matrix/matrix_integer_dense.pyx prevents it from compiling.

Upstream: Reported upstream. No feedback yet.

CC: @ClementPernet

Component: interfaces

Keywords: sd75

Author: Charles Bouillaguet

Branch/Commit: 08277ea

Reviewer: Jeroen Demeyer

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

@sagetrac-Bouillaguet sagetrac-Bouillaguet mannequin added this to the sage-7.4 milestone Aug 26, 2016
@sagetrac-Bouillaguet

This comment has been minimized.

@sagetrac-Bouillaguet
Copy link
Mannequin Author

sagetrac-Bouillaguet mannequin commented Aug 26, 2016

Changed keywords from none to sd75

@sagetrac-Bouillaguet

This comment has been minimized.

@sagetrac-Bouillaguet
Copy link
Mannequin Author

sagetrac-Bouillaguet mannequin commented Aug 27, 2016

Branch: u/Bouillaguet/iml_wrapper

@sagetrac-Bouillaguet
Copy link
Mannequin Author

sagetrac-Bouillaguet mannequin commented Aug 27, 2016

Commit: 574b438

@sagetrac-Bouillaguet
Copy link
Mannequin Author

sagetrac-Bouillaguet mannequin commented Aug 27, 2016

New commits:

574b438move access to IML code into sage.libs.iml

@jdemeyer
Copy link

comment:5

What is the point of this:

cdef long iml_nullspaceMP(long n, long m, const mpz_t *A, mpz_t * *mp_N_pass):
    return nullspaceMP(n, m, A, mp_N_pass)

cdef iml_nonsingSolvLlhsMM(SOLU_POS solupos, long n, long m, mpz_t *mp_A, mpz_t *mp_B, mpz_t mp_N, mpz_t mp_D):
    nonsingSolvLlhsMM(solupos, n, m, mp_A, mp_B, mp_N, mp_D)

Why not just call the IML functions directly?

If it's related to the problem in the ticket description, I'd rather try to fix that problem or at least understand why it does not work.

@jdemeyer

This comment has been minimized.

@jdemeyer

This comment has been minimized.

@jdemeyer
Copy link

comment:8

I think this is an upstream linbox bug.

@jdemeyer

This comment has been minimized.

@jdemeyer
Copy link

Upstream: Reported upstream. No feedback yet.

@jdemeyer
Copy link

comment:10

In code like

cdef enum SOLU_POS:
        LeftSolu = 101
        RightSolu = 102

it's better to not put the values (101 and 102) since this confuses people to think that Cython actually uses those values (it just ignores them). Also: use 4 spaces of indentation.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 28, 2016

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

08277eamove access to IML code into sage.libs.iml

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 28, 2016

Changed commit from 574b438 to 08277ea

@sagetrac-Bouillaguet
Copy link
Mannequin Author

sagetrac-Bouillaguet mannequin commented Aug 28, 2016

comment:12

Jeroen, does this suit you (also see my "private" email)?

@jdemeyer
Copy link

comment:13

Let's see what the patchbot says (you can remind me if the patchbot tests pass).

@jdemeyer
Copy link

comment:14

And fill in your name as author...

@sagetrac-Bouillaguet
Copy link
Mannequin Author

sagetrac-Bouillaguet mannequin commented Aug 30, 2016

Author: Charles Bouillaguet

@sagetrac-Bouillaguet
Copy link
Mannequin Author

sagetrac-Bouillaguet mannequin commented Aug 30, 2016

comment:15

Oops.

@sagetrac-Bouillaguet
Copy link
Mannequin Author

sagetrac-Bouillaguet mannequin commented Sep 1, 2016

comment:16

Jeroen, the tests fail on the patchbot for an unrelated reason (a deprecation warning in generic_graph.pyx). I can't reproduce the problem on my machine. How do you feel about the ticket?

@jdemeyer
Copy link

jdemeyer commented Sep 1, 2016

comment:17

The patchbot failures are indeed unrelated.

@jdemeyer
Copy link

jdemeyer commented Sep 1, 2016

Reviewer: Jeroen Demeyer

@vbraun
Copy link
Member

vbraun commented Sep 4, 2016

Changed branch from u/Bouillaguet/iml_wrapper to 08277ea

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants