allow algorithm=pari in LLL #37207

AurelPage · 2024-01-31T10:30:04Z

We allow to call the pari implementation of LLL reduction. The next release of pari will contain an implementation of the FLATTER algorithm for LLL reduction, which has better asymptotic complexity.

📝 Checklist

The title is concise, informative, and self-explanatory.
The description explains in detail what this PR is about.
I have linked a relevant issue or discussion.
I have created tests covering the changes.
I have updated the documentation accordingly.

seblabbe · 2024-01-31T15:13:51Z

src/sage/matrix/matrix_integer_dense.pyx

+            sage: M.rank()
+            2
+
+            sage: M = matrix(4,3,[1,2,3,2,4,6,7,0,1,-1,-2,-3])


This repeat the line above: remove.

seblabbe · 2024-01-31T15:14:06Z

src/sage/matrix/matrix_integer_dense.pyx

+            sage: M.rank()
+            2
+
+            sage: M = matrix(4,3,[1,2,3,2,4,6,7,0,1,-1,-2,-3])


This repeat the line above: remove

seblabbe · 2024-01-31T15:14:34Z

src/sage/matrix/matrix_integer_dense.pyx

+            sage: M.LLL()[0:2]
+            [0 0 0]
+            [0 0 0]
+            sage: M.rank()


I would remove the M.rank() line from this example.

seblabbe · 2024-01-31T15:15:49Z

src/sage/matrix/matrix_integer_dense.pyx

+            sage: M.LLL(algorithm="NTL:LLL")[0:2]
+            [0 0 0]
+            [0 0 0]
+            sage: M.rank()


I suggest to add a doctest block in the TESTS:: part which test that the rank is computed correctly by each algo with proper M._clear_cache() calls in between.

seblabbe · 2024-01-31T15:30:23Z

src/sage/matrix/matrix_integer_dense.pyx

+        elif algorithm == 'pari':
+            # call pari with flag=4: kernel+image
+            # pari uses column convention: need to transpose the matrices
+            from sage.libs.pari import pari


if we use self._gp_().mattranspose() below, then, we don't need to import pari on this line

Also we would need to use K.sage().augment(T.sage()) or something like this.

I don't have a strong opinion on this. We may keep it as it is if it is better overall.

I'm not sure about this one. Maybe someone else has an opinion?

Never use _gp_, which is going through the old and slow interface ; use __pari__

sage: m=matrix(ZZ,4,4,range(16)) sage: type(m) <class 'sage.matrix.matrix_integer_dense.Matrix_integer_dense'> sage: m._gp_() [0, 1, 2, 3; 4, 5, 6, 7; 8, 9, 10, 11; 12, 13, 14, 15] sage: type(_) <class 'sage.interfaces.gp.GpElement'> sage: m.__pari__() [0, 1, 2, 3; 4, 5, 6, 7; 8, 9, 10, 11; 12, 13, 14, 15] sage: type(_) <class 'cypari2.gen.Gen'>

Ok, thanks! Would it be better to use pari to avoid the import statement?

I don't really see a reason to avoid the import statement: It's essentially free since the library has already been loaded anyway.

sage: %timeit from sage.libs.pari import pari 182 ns ± 1.79 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each)

Shouldn't this be using cypari2, not pari ?

What do you mean?

>>> from sage.libs.pari import pari >>> type(pari) <class 'cypari2.pari_instance.Pari'>

oops, never mind, sorry

Though, for the conversion, it is probably faster to go through M.__pari__() or even the more direct

cdef Gen pari_matrix = integer_matrix(self._matrix, 0)

(which is the one-line code of M.__pari__()). The second argument of integer_matrix allows you to transpose which might be useful here!

To convert back from pari the most direct at the moment is to call gen_to_sage_matrix from sage.libs.pari.convert_sage_matrix. Though this function does not take into account the fact that the entries are integral and there is a bunch of irrelevant python objects built for each entry. For huge matrices it could matter.

I will propose a commit.

seblabbe · 2024-01-31T15:31:35Z

src/sage/matrix/matrix_integer_dense.pyx

+            K, T = A.qflll(4)
+            U = pari.matconcat([K, T]).mattranspose().sage()
+            R = U * self
+            r = ZZ(T.length())


This is a pari method call done after lines where the sage language is performed. Maybe keep pari computations all together is more readable.

src/sage/matrix/matrix_integer_dense.pyx

seblabbe

Looks good to me.

dimpase · 2024-02-01T13:51:45Z

@videlec

Co-authored-by: Sébastien Labbé <seblabbe@users.noreply.github.com>

videlec · 2024-02-03T18:13:52Z

Hmm... caching in LLL is not very smart

sage: m = matrix(ZZ, 2, 2, [-1,1,1,1])
sage: m.LLL(algorithm="NTL:LLL")
[-1  1]
[ 1  1]
sage: m.det()
2
sage: m = matrix(ZZ, 2, 2, [-1,1,1,1])
sage: m.det()
-2

Now #37236

videlec · 2024-02-03T18:44:14Z

@AurelPage
I can not push to your branch. github is really bad with collaborations. I added an extra commit to my fork of sage: https://github.com/videlec/sage/tree/pari-LLL

AurelPage · 2024-02-03T19:09:03Z

@videlec Thanks! I am trying your commit. Git made decentralised collaboration easy, and then github ruined it.

AurelPage · 2024-02-03T19:15:09Z

@videlec I've included your commit.

github-actions · 2024-02-03T19:48:03Z

Documentation preview for this PR (built with commit c89b30a; changes) is ready! 🎉

dimpase · 2024-02-03T20:00:57Z

@AurelPage
I can not push to your branch. github is really bad with collaborations. I added an extra commit to my fork of sage: https://github.com/videlec/sage/tree/pari-LLL

I think this "cannot push to your branch" only happens if the PR's author disallowed such pushes while creating the PR (not sure if this can be changed later), or if the contributor who wants add commits is not a member of Sagemath Triage team.

In any event it should always be possible to make a PR against the fork of this PR on the PR's author repo (and then PR's author can use GitHub's interface to deal with it).

I agree that's confusing, and could be documented better.

AurelPage · 2024-02-03T20:07:48Z

@dimpase I allowed edits by maintainers, if that's what you mean. And it can be changed after the PR has been created.

dimpase · 2024-02-03T20:10:04Z

perhaps @videlec is not a member of Triage team? (I can't check this myself)

videlec · 2024-02-03T20:18:19Z

Indeed, push are allowed for "maintainers" but not anybody (like me :-)

dimpase · 2024-02-03T20:45:26Z

pushes are not allowed for just anybody, for a good reason - to prevent vandalism from persons unaffiliated with the project.

In this case "maintainer" means being on an appropriate Sagemath (a GitHub organisation) team, with appropriate push access. I thought that members of Triage have such rights - to push to the branches related to the PRs (not to protected branches) - but I might be wrong here.

Maybe @vbraun knows better.

In any case, asking the author of the PR to manually merge a commit looks like a suboptimal thing, going via a PR to the fork looks more natural.

We allow to call the pari implementation of LLL reduction. The next release of pari will contain an implementation of the FLATTER algorithm for LLL reduction, which has better asymptotic complexity.    ### 📝 Checklist     - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [ ] I have linked a relevant issue or discussion. - [x] I have created tests covering the changes. - [x] I have updated the documentation accordingly.  URL: sagemath#37207 Reported by: AurelPage Reviewer(s): AurelPage, Dima Pasechnik, Frédéric Chapoton, Lorenz Panny, Sébastien Labbé, Vincent Delecroix

AurelPage added sd125 sage days 125 c: linear algebra s: needs review labels Jan 31, 2024

AurelPage requested review from alexjbest, xcaruso and seblabbe January 31, 2024 14:27

seblabbe reviewed Jan 31, 2024

View reviewed changes

src/sage/matrix/matrix_integer_dense.pyx Outdated Show resolved Hide resolved

seblabbe approved these changes Jan 31, 2024

View reviewed changes

AurelPage removed request for alexjbest and xcaruso February 2, 2024 08:23

AurelPage and others added 3 commits February 3, 2024 13:28

allow algorithm=pari in LLL

bc085f6

pari's LLL: improve doc and tests

d7a1ab2

fix tab

2a5fd15

Co-authored-by: Sébastien Labbé <seblabbe@users.noreply.github.com>

AurelPage force-pushed the pari-LLL branch from 9d8f441 to 2a5fd15 Compare February 3, 2024 12:28

smarter pari conversions and factorize caching

c89b30a

videlec added s: positive review and removed s: needs review labels Feb 4, 2024

vbraun merged commit bf1c793 into sagemath:develop Feb 13, 2024
15 of 19 checks passed

github-actions bot removed the s: positive review label Feb 14, 2024

mkoeppe added this to the sage-10.3 milestone Mar 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow algorithm=pari in LLL #37207

allow algorithm=pari in LLL #37207

AurelPage commented Jan 31, 2024

seblabbe Jan 31, 2024

seblabbe Jan 31, 2024

seblabbe Jan 31, 2024

seblabbe Jan 31, 2024

seblabbe Jan 31, 2024

AurelPage Jan 31, 2024

fchapoton Jan 31, 2024

AurelPage Jan 31, 2024

yyyyx4 Jan 31, 2024

videlec Feb 1, 2024

dimpase Feb 3, 2024

videlec Feb 3, 2024

videlec Feb 3, 2024

videlec Feb 3, 2024

seblabbe Jan 31, 2024

seblabbe left a comment

dimpase commented Feb 1, 2024

videlec commented Feb 3, 2024 •

edited

videlec commented Feb 3, 2024 •

edited

AurelPage commented Feb 3, 2024

AurelPage commented Feb 3, 2024 •

edited

github-actions bot commented Feb 3, 2024

dimpase commented Feb 3, 2024 •

edited

AurelPage commented Feb 3, 2024

dimpase commented Feb 3, 2024

videlec commented Feb 3, 2024

dimpase commented Feb 3, 2024

allow algorithm=pari in LLL #37207

allow algorithm=pari in LLL #37207

Conversation

AurelPage commented Jan 31, 2024

📝 Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seblabbe left a comment

Choose a reason for hiding this comment

dimpase commented Feb 1, 2024

videlec commented Feb 3, 2024 • edited

videlec commented Feb 3, 2024 • edited

AurelPage commented Feb 3, 2024

AurelPage commented Feb 3, 2024 • edited

github-actions bot commented Feb 3, 2024

dimpase commented Feb 3, 2024 • edited

AurelPage commented Feb 3, 2024

dimpase commented Feb 3, 2024

videlec commented Feb 3, 2024

dimpase commented Feb 3, 2024

videlec commented Feb 3, 2024 •

edited

videlec commented Feb 3, 2024 •

edited

AurelPage commented Feb 3, 2024 •

edited

dimpase commented Feb 3, 2024 •

edited