LLL_gram, clarifying and handling undefined behavior when self is not positive definite

[edgarcosta]

I tried to make it clear that self must be positive definite and showed some examples where things can go awry if that is not the case.

I tempted to mention LLLGram in magma that seems to address this issues, and might a nice alternative through magma_free

map(eval, magma_free("G, U, r := LLLGram(Matrix(2,2,[0,1,1,0])); print ElementToSequence(G); print ElementToSequence(U); print r;").splitlines())

Component: linear algebra

Keywords: days88

Author: Edgar Costa

Branch/Commit: 2e12286

Reviewer: Aly Deines

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

[fchapoton]

comment:2

Doc does not build :

+[dochtml] [matrices ] docstring of sage.matrix.matrix_integer_dense.Matrix_integer_dense.LLL_gram:3: ERROR: Unexpected indentation.
+[dochtml] [matrices ] docstring of sage.matrix.matrix_integer_dense.Matrix_integer_dense.LLL_gram:11: WARNING: Bullet list ends without a blank line; unexpected unindent.

You need to respect the doc syntax (http://doc.sagemath.org/html/en/developer/coding_basics.html).

[sagetrac-git]

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

352cf22

fixing doc build

[sagetrac-git]

Changed commit from 9e58841 to 352cf22

[fchapoton]

comment:5

there should be an empty line after .. WARNING::

and for the reference to pari, you can use the trac role, that creates an hyperlink:

:pari:`qfllgram`

[fchapoton]

comment:6

in the doctests, the syntax is

here we go::

    sage: 2+2
    4

you forgot the double colon and the empty line just after it.

[sagetrac-git]

Changed commit from 352cf22 to a27fde1

[sagetrac-git]

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

a27fde1

fixing doc syntax

[sagetrac-git]

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

1de49f6

empty lines

[sagetrac-git]

Changed commit from a27fde1 to 1de49f6

[sagetrac-git]

Changed commit from 1de49f6 to 1cb8f35

[sagetrac-git]

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

1cb8f35

reference pari

[edgarcosta]

comment:10

Thanks for the tips!
I'm now double checking that everything builds fine.

Only after doing make doc-clean I managed to see the errors that you mentioned.

[sagetrac-git]

Changed commit from 1cb8f35 to cea589e

[sagetrac-git]

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

cea589e

a missing fake traceback

[edgarcosta]

comment:12

I think it is all good now.

[fchapoton]

comment:13

There is a mixture of qfllgram and qflllgram. How many l's are there ?

[sagetrac-git]

Changed commit from cea589e to 56763a1

[sagetrac-git]

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

436614c	Merge branch 'u/edgarcosta/LLL_gram' of git://trac.sagemath.org/sage into develop
56763a1	fixing the number of ls in qflllgram

[edgarcosta]

comment:15

I have fixed the number of l's.

Sorry for the delay, totally missed your comment.

Cheers,
Edgar

[sagetrac-git]

Changed commit from 56763a1 to 96efbcb

[sagetrac-git]

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

96efbcb

Merge branch 'u/edgarcosta/LLL_gram' of git://trac.sagemath.org/sage into t/23581/edgarcosta

[sagetrac-git]

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

172c505

formatting

[sagetrac-git]

Changed commit from 96efbcb to 172c505

[sagetrac-git]

Changed commit from 172c505 to 73ae7cf

[sagetrac-git]

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

73ae7cf

word wrapping

[adeines]

Reviewer: Aly Deines

[adeines]

comment:19

Looks good. Just a few changes.

I think

sage: Matrix(ZZ, [-5, -1, -1, -5]).LLL_gram() # not tested

should be

sage: Matrix(ZZ, 2, 2, [-5, -1, -1, -5]).LLL_gram() # not tested

If you change that and remove all trailing whitespace, I'll give you a positive review.

[sagetrac-git]

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

2e12286

some more formatting

[sagetrac-git]

Changed commit from 73ae7cf to 2e12286

[edgarcosta]

Changed keywords from none to days88

[adeines]

comment:22

Thanks!

[vbraun]

Changed branch from u/edgarcosta/LLL_gram to 2e12286