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

Householder inside LLL #361

Merged
merged 164 commits into from Dec 5, 2018

Conversation

Projects
None yet
2 participants
@lgremy
Contributor

lgremy commented Nov 26, 2018

Purpose of the pull request

This pull request aims to provide an implementation of Householder inside LLL (HLLL). This implementation comes with 3 main methods (in the sens of -m in fplll) that can be called thanks to the fplll -a hlll -m [method]

  • fast: similar to the one of -a lll, i.e., the ouput basis is not possibly not HLLL-reduced.
  • proved: only tested with mpfr, the output basis is HLLL-reduced.
  • wrapper: not as exhaustive as in LLL, basically begins the reduction with double, then switch to longdouble if the tests to detect not enough precision are reached with double and so one to increase the precision.

Efficiency

Compared to the LLL implementation, here are some timings. The benchmarks are done on an Intel(R) Core(TM) i5-7500 CPU @ 3.40GHz, with fplll compiled with -Wall -g -Wextra -Wpedantic -pedantic -O3 and gcc (Debian 8.2.0-9) 8.2.0. We run the command

for n in {100,128,140,160,180,200,240,280,300,400}
do
    for seed in `seq 0 1 5`
    do
        n=$n seed=$seed para=true bash test_hlll_vs_lll.sh
    done
done

where test_hlll_vs_lll.sh . The tests are done on (only) six random knapsack like lattices of dimension n and entry of bit-length <= n * 100.

n 100 128 140 160 180 200 240 280 300 400 (five computations)
lll (s) 45 142 228 483 916 1544 3896 8674 11860 62383
hlll (s) 61 179 270 519 886 1436 3419 7803 10777 47235

For a comparison with another implementation of HLLL, see Gilles Villard's tests.

Known limitations

With fast, the output basis is not always HLLL-reduced w.r.t delta. Indeed, if we want to have a (delta, eta, theta)-weak size-reduced basis, the parameter delta must be increased (e.g., if the output basis must be delta=0.99 reduced, the parameter delta must be set to 0.991 or 0.992). This is inherent with HLLL, but must be take into consideration before running a computation.

The wrapper is not as exhaustive as for LLL.

hplll seems a bit faster than this implementation of HLLL, meaning probably that some ideas (especially in the update_R function in fplll which correspond to householder_r in hplll) can be reproduced.

@@ -176,8 +176,9 @@ The options are:
* `-a sld` : slide reduction.
* `-a cvp` : prints the vector in the lattice closest to the input vector.
* `-v` : verbose mode.
* `-nolll` : does not apply to LLL-reduction. In the case of bkz, hkz and svp, by default, the input basis is LLL-reduced before anything else. This option allows to remove that initial LLL-reduction (note that other calls to LLL-reduction may occur during the execution).
* `-nolll` : does not apply to LLL-reduction. In the case of bkz, hkz and svp, by default, the input basis is LLL-reduced before anything else. This option allows to remove that initial LLL-reduction (note that other calls to LLL-reduction may occur during the execution). In the cas of hlll, verify if the input basis is HLLL-reduced.

This comment has been minimized.

@malb

malb Nov 26, 2018

Collaborator

I don't understand what this means.

This comment has been minimized.

@lgremy

lgremy Nov 26, 2018

Contributor

I will take an example: calling fplll -a hlll -nolll [somebasis] will not HLLL-reduce the basis, but just check if [somebasis] is HLLL-reduced or not. It is a way to only call is_hlll_reduced from the binary. I can rewrite this part to be more precise.

This comment has been minimized.

@malb

malb Nov 27, 2018

Collaborator

Does it have the same behaviour when we do -a lll -nolll, i.e. is that consistent across the two LLLs?

This comment has been minimized.

@lgremy

lgremy Nov 27, 2018

Contributor

It is inconsistent. fplll -a lll -nolll skip -nolll and perform the same thing as fplll -a lll (as I can observe), then apply an LLL-reduction on the input basis.

Show resolved Hide resolved fplll/defs.h
Show resolved Hide resolved fplll/gso.cpp Outdated
template <class ZT, class FT> bool HLLLReduction<ZT, FT>::hlll()
{
/* TODO: we do not use a completely correct value for delta. We must use a value

This comment has been minimized.

@malb

malb Nov 26, 2018

Collaborator

What is the status of this TODO.

This comment has been minimized.

@lgremy

lgremy Nov 26, 2018

Contributor

This TODO is related to the first limitation I listed in the comment of the PR. Indeed, in the paper that introduce HLLL, the delta to do the computation must be in an interval (delta_input + 2^(-p + p0), 1 - 2^(-p + p0)). I presume this is more or less the same in LLL, delta must be a bit modified to guarantee the computation. It is a TODO for a careful analysis about how to set delta to guarantee the HLLL-reduction w.r.t delta.

m.norm_square_R_row(ftmp1, k, k, m.get_n(),
expo1); // sum_{i = k}^{i < n}R[k][i]^2 = ftmp1 * 2^expo1
#ifdef DEBUG
m.get_R(ftmp0, k, k - 1, expo0); // R(k, k - 1) = ftmp0 * 2^expo0

This comment has been minimized.

@malb

malb Nov 26, 2018

Collaborator

Why does this depend on DEBUG?

This comment has been minimized.

@lgremy

lgremy Nov 26, 2018

Contributor

I modified the code in bad95bd to explain why this depend on DEBUG. Maybe it is not a good idea, since returning or not expo0 is not related to an efficiency problem.

Show resolved Hide resolved fplll/hlll.h Outdated
@@ -0,0 +1,590 @@
/* Copyright (C) 2005-2008 Damien Stehle.
Copyright (C) 2007 David Cade.
Copyright (C) 2011 Xavier Pujol.

This comment has been minimized.

@malb

malb Nov 26, 2018

Collaborator

You are missing here.

This comment has been minimized.

@lgremy

lgremy Nov 26, 2018

Contributor

Yes. In fact, I removed these lines from all the other files I have introduced. I am not really I big fan of putting my name in the beginning of the file (in fact, I am not sure that the naming convention in fplll is respected). I understand that this can be an issue for changing the license or other related things, then I may follow the rules for these reasons. What do you propose?

This comment has been minimized.

@malb

malb Nov 27, 2018

Collaborator

I don't think you can remove them as these are legal statements. I'm not a lawyer, though. I agree that we do have copyright information in the form of git commits but I think it's still a good idea to claim copyright when having done something major. Thus, I'd propose that you add yourself. Indeed, if we wanted to change the license we'd have to reach out to all copyright holders.

This comment has been minimized.

@lgremy

lgremy Nov 27, 2018

Contributor

Since I have introduced this file (I just brainless copy the header of gso.cpp), this three first lines can maybe just be dropped, since they were not written by any of them (at least, I take the idea of using row_expo in gso.cpp). But I am also not a lawyer, and not aware about how we should proceed. I will follow your advice.

This comment has been minimized.

@malb

malb Nov 27, 2018

Collaborator

Agreed, these lines should go here. Taking an idea from somewhere is not the same as copyright which about the literal code, not an idea in the code.

This comment has been minimized.

@lgremy

lgremy Nov 28, 2018

Contributor

Add copyright information with commit 94aba9b.

else
{
// Simply copy b[i] in bf[i]
// TODO: these are row operations. However, since the types are converted, maybe difficult

This comment has been minimized.

@malb

malb Nov 26, 2018

Collaborator

What is the status of this TODO?

This comment has been minimized.

@lgremy

lgremy Nov 26, 2018

Contributor

At one point, I decided to try to write row operations in matrix.h instead of explicitly write the for loop on the element in householder.cpp. However, some of these operations can be shared by other part of the code in fplll, in gso* for example, and I do not take the time to factorize all the code at this step, but this is a project I plan to do in a near future. So for now, this is still a TODO.

/** Computes the truncated dot product between two rows of a Matrix.
* Constraint: n > beg.
*/
inline void dot_product(T &result, const MatrixRow<T> &v0, int beg, int n) const

This comment has been minimized.

@malb

malb Nov 26, 2018

Collaborator

Ah, so this PR modifies the interface of dot_product.

This comment has been minimized.

@lgremy

lgremy Nov 26, 2018

Contributor

Yes, and I realize this is not a good idea. There is obviously a way to keep a function dot_product(T &, const MatrixRow<T> &, int n) by specializing dot_product(T &, const MatrixRow<T> &, 0, int n). This had the advantage to not break the API, especially for fpylll.

This comment has been minimized.

@lgremy

lgremy Nov 28, 2018

Contributor

I have reintroduced the previous way to call dot_productin commit 5330e00.

@@ -210,7 +286,7 @@ template <class ZT, class FT> int test_int_rel(int d, int b)
<< " shows different GSO-outputs for grammatrix representation and basis representation.\n";
return 1;
}
return 0;
return test_householder<ZT, FT>(A);

This comment has been minimized.

@malb

malb Nov 26, 2018

Collaborator

Maybe do this call in the same style as the above calls to keep the symmetry?

This comment has been minimized.

@lgremy

lgremy Dec 4, 2018

Contributor

This must be done in commit 41fb850.

@codecov

This comment has been minimized.

codecov bot commented Nov 26, 2018

Codecov Report

Merging #361 into master will increase coverage by 0.46%.
The diff coverage is 75.92%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #361      +/-   ##
==========================================
+ Coverage   72.12%   72.58%   +0.46%     
==========================================
  Files          62       66       +4     
  Lines        5392     6107     +715     
==========================================
+ Hits         3889     4433     +544     
- Misses       1503     1674     +171
Impacted Files Coverage Δ
fplll/wrapper.h 100% <ø> (ø) ⬆️
fplll/util.h 100% <ø> (ø) ⬆️
fplll/util.cpp 88.63% <100%> (+6.28%) ⬆️
fplll/nr/numvect.h 82.14% <100%> (+3.29%) ⬆️
fplll/nr/matrix.h 71.92% <100%> (+5.89%) ⬆️
fplll/wrapper.cpp 60.69% <55.82%> (-3.76%) ⬇️
fplll/hlll.h 58.97% <58.97%> (ø)
fplll/householder.h 68.64% <68.64%> (ø)
fplll/householder.cpp 82.19% <82.19%> (ø)
fplll/hlll.cpp 92.06% <92.06%> (ø)
... and 12 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 a075574...8400e7d. Read the comment docs.

@malb

This comment has been minimized.

Collaborator

malb commented Nov 26, 2018

From reading the PR I gather there's no way this could be integrated more with the rest of the library, i.e. it's futile to attempt to build BKZ on top of it as it operates on MatHousholder instead of MatGSO. Or can those be made to work to use the MatGSOInterface and then there's also an LLLReductionInterface?

@lgremy

This comment has been minimized.

Contributor

lgremy commented Nov 26, 2018

Edit

I am not completely sure that this will be impossible (in fact, what you ask was one of the Damien's requirements when I began this work). However, I think such a change will probably modify the API of fplll. I will give my two cents about this, but I think this question is large enough to be discussed in a new Issue.

  1. HLLL inside BKZ

It seems an option to have HLLL inside BKZ. However, I do not know if the efficiency of HLLL can outperform the one of LLL with the actual block-sizes of BKZ.

  1. About LLLReduction

LLLReduction is composed of two public methods: lll and size_reduction. A number of private / public variables or methods can be shared in common in an LLLReductionInterface between LLLReduction and HLLLReduction (we must however think about how to provide a compact and clean API). Concerning the HLLLReduction, It will be needed to adapt the functions to perform HLLL in order to have the same behavior related to LLL and HLLL, but this seems to be not a big deal.

  1. About MatGSOInterface

By looking at bkz.cpp, I realized that a number of needed operations on a MatGSO (since only MatGSO seems to be used in BKZ) to perform BKZ are not provided. It is maybe not a big deal, but I cannot have a clear idea for now. However, it seems to me impossible that MatHouseholder implements MatGSOInterface, since the internal variable needed by MatGSOInterface to perform some top-level methods are not available in MatHouseholder, and must not in my point of view. To have a common interface, we need to redesign all the MatGSO* stuff, and also MatHouseholder.

This opens a large number of questions on the mathematical and software engineering levels, at least.

@malb

This comment has been minimized.

Collaborator

malb commented Nov 27, 2018

On HLLL in BKZ: Agreed, we'd need HLLL on a sublattice. I wouldn't worry about detecting insufficient precision (except for infinite loops) within BKZ, it is assumed that the initial (H)LLL object is constructed with enough precision to run. Note that (H)LLL in BKZ runs over the whole basis from time to time, not only in the block. Thus, it would help for bases of dimension, say, 250.

Your plans for LLLReductionInterface sound good, I'd vote for something like that.

@malb

This comment has been minimized.

Collaborator

malb commented on fplll/hlll.cpp in 2255ff6 Nov 29, 2018

maintened -> maintained

@lgremy

This comment has been minimized.

Contributor

lgremy commented Dec 1, 2018

Your plans for LLLReductionInterface sound good, I'd vote for something like that.

I started doing this in the branch hlll_in_bkz in my repository. Since it will modify the API of LLLReduction, I suggest that this change was referenced as an Issue once this PR will be closed, with a reference to the discussions we had here.

@malb

This comment has been minimized.

Collaborator

malb commented Dec 2, 2018

To make sure I understand. You propose to add the HLLLReduction class as is for now and do the LLLReductionInterface change in a later pull request? I think we'd want to make sure not to release in between, in that case, i.e. not introduce a temporary API. However, we don't release that often anyway.

@lgremy

This comment has been minimized.

Contributor

lgremy commented Dec 3, 2018

Yes. If you agree, I prefer to have the HLLLReduction class as is for now, if possible.

Indeed, the modifications we need to do, in order to provide a common interface LLLReductionInterface shared between LLLReduction and HLLLReduction, change the current API and are at least difficult to test if HLLLReduction is not used inside BKZ. For now, in the branch hlll_in_bkz, the implemented methods to respect the proto-LLLReductionInterface class are sadly not tested (explicitly written in the comments).

However, to fully integrate HLLL inside BKZ, we also need to have a common interface between MatGSO and MatHouseholder, which will also change the API, and is much more difficult to do than for LLLReductionInterface.

If you think that it will be a good idea to have at least the LLLReductionInterface class to close this PR, I suggest we take a break with this PR to have time to design properly the new API and test the HLLLReduction methods with their new features.

@malb

This comment has been minimized.

Collaborator

malb commented Dec 3, 2018

Okay, let's do it like this: merge this PR once the last details are addressed (e.g. it seems coverage has gone down, did you check?) and then do a separate proper-API PR.

@lgremy

This comment has been minimized.

Contributor

lgremy commented Dec 4, 2018

Okay, now, all the lights are green. I tried to address all the points mentioned in the conversations. If you see other improvements or modifications I forgot, please let me know.

@malb malb changed the base branch from master to hlll Dec 5, 2018

@malb malb merged commit a8cb572 into fplll:hlll Dec 5, 2018

3 checks passed

codecov/patch 75.92% of diff hit (target 50%)
Details
codecov/project 72.58% (+0.46%) compared to a075574
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment