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

Added givens #351

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Added givens #351

wants to merge 4 commits into from

Conversation

kodebro
Copy link
Contributor

@kodebro kodebro commented Feb 19, 2018

What's new

  • Added gso_givens.h and gso_givens.cpp.
  • Added test_lll_givens.cpp in the tests folder.

The givens GSO class should be compatible with LLL, but not (yet) with Wrapper. I didn't check whether BKZ can run on this givens GSO class.

Issues:

  • Operations on the Givens-matrix l_givens are mainly Givens-rotations, which are column operations. @lducas suggested to store the transpose of l_givens instead of l_givens itself. However, then the 'lazy' size-reductions occur as column operations. So, the main question is: Should we store l_givens or the transpose of it? What is the fastest way?
  • Discovering rows could be amortized by discovering multiple rows at a time, and not telling it to the interface.
  • How do the different lazyness approaches influence the numerical stability?

Speed

  • The Givens-GSO is [compared with the same precision] 2 - 3 times slower than the normal GSO. At dimension 180 however, the normal GSO aborts due to instability, while Givens proceeds.
  • There's a test-file test_benchmark.cpp which tests various big cases. I will remove this one -- if you want to check for the timings yourself, run it separately.

@codecov
Copy link

codecov bot commented Mar 9, 2018

Codecov Report

Merging #351 into master will decrease coverage by 0.53%.
The diff coverage is 51.73%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #351      +/-   ##
==========================================
- Coverage   74.36%   73.82%   -0.54%     
==========================================
  Files          55       57       +2     
  Lines        4891     5211     +320     
==========================================
+ Hits         3637     3847     +210     
- Misses       1254     1364     +110
Impacted Files Coverage Δ
fplll/gso_interface.h 41.44% <ø> (-0.91%) ⬇️
fplll/gso_givens.h 33.33% <33.33%> (ø)
fplll/gso_givens.cpp 56.29% <56.29%> (ø)
fplll/lll.cpp 93.96% <0%> (+2.51%) ⬆️
fplll/nr/matrix.h 69.09% <0%> (+3.05%) ⬆️
fplll/nr/nr_Z_mpz.inl 92.59% <0%> (+8.64%) ⬆️
fplll/nr/numvect.h 90.47% <0%> (+10.86%) ⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 661daa8...73a552c. Read the comment docs.

@lducas
Copy link
Member

lducas commented Jun 3, 2018

Ping ?

@shi-bai
Copy link
Collaborator

shi-bai commented Jun 11, 2018

I can try to look at this. @lgremy would you mind helping with this as well?

@shi-bai shi-bai assigned shi-bai and lgremy and unassigned shi-bai and lgremy Jun 12, 2018
@shi-bai shi-bai requested review from shi-bai and lgremy June 12, 2018 12:23
@lgremy
Copy link
Contributor

lgremy commented Sep 26, 2018

Hi,

Sorry for the very late reply. I do not have time to look carefully at the code, but I have two remarks, especially about the running time and the lazyness approach.

  1. Running time
    I do not think that we can compare timing at the point of the implementation. Indeed, the variable n_known_cols is not use in this implementation. The goal of n_known_cols in the classical GSO implementation is to try to capture the shape of the basis and avoid to do computation of the type a += 0 * 0. It can be a part of the explanation of

The Givens-GSO is [compared with the same precision] 2 - 3 times slower than the normal GSO.

  1. Lazyness approach
    I do not have a reference about the use of Givens inside LLL. But if I compare with Householder inside LLL, which must provide more or less the same numerical stability, the R coefficients are refreshed at each incomplete size reduction (Step 2 of Algorithm 3). So I suppose that for Givens, it must be quite the same.

Householder inside LLL is supposed to be faster than LLL using GSO for dimensions larger than 160-170 (see Gilles Villard's tests), so I will be not surprised if it would be the same for Givens, and this is what we can expect from your observation

At dimension 180 however, the normal GSO aborts due to instability, while Givens proceeds.

I will continue to look at the code, but is there any reference describing how to use Givens inside LLL?

Thanks.

@kodebro
Copy link
Contributor Author

kodebro commented Oct 1, 2018

Dear @lgremy, (and others)

I am sorry for leaving out the most important part [comments/references] of the Givens implementation. Next week I try to find time to look at my code again and putting appropriate examples, references and comments.

Koen

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

Successfully merging this pull request may close these issues.

4 participants