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.

Sign up for GitHub

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

Jump to bottom

Fix inner sum in BGV #513

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Fix inner sum in BGV #513

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
529dcd5
fix innersum bgv
lehugueni Nov 19, 2024
141c402
tests + use maxslots instead of N
lehugueni Nov 21, 2024
b67c72b
return ctIn when n=1, batchSize=k*N
lehugueni Nov 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 75 additions & 0 deletions schemes/bgv/bgv_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (

"github.com/tuneinsight/lattigo/v6/core/rlwe"
"github.com/tuneinsight/lattigo/v6/ring"
"github.com/tuneinsight/lattigo/v6/utils"
)

var flagPrintNoise = flag.Bool("print-noise", false, "print the residual noise")
Expand Down Expand Up @@ -665,6 +666,80 @@ func testEvaluatorBvg(tc *TestContext, t *testing.T) {
}
})
}

// Naive implementation of the inner sum for reference
innersum := func(values []uint64, n, batchSize int) {
tmp := make([]uint64, len(values))
copy(tmp, values)
for i := 1; i < n; i++ {
rot := utils.RotateSlice(tmp, i*batchSize)
for j := range values {
values[j] = (values[j] + rot[j]) % tc.Params.PlaintextModulus()
}
}
}

for _, N := range []int{tc.Params.N(), tc.Params.MaxSlots()} {
for _, lvl := range testLevel {
t.Run(name("Evaluator/InnerSum/N slots", tc, lvl), func(t *testing.T) {
if lvl == 0 {
t.Skip("Skipping: Level = 0")
}
n := N >> 2
batchSize := 1 << 2

galEls := tc.Params.GaloisElementsForInnerSum(batchSize, n)
evl := tc.Evl.WithKey(rlwe.NewMemEvaluationKeySet(nil, tc.Kgen.GenGaloisKeysNew(galEls, tc.Sk)...))

want, _, ciphertext0 := NewTestVector(tc.Params, tc.Ecd, tc.Enc, lvl, tc.Params.NewScale(3))

innersum(want, n, batchSize)

receiver := NewCiphertext(tc.Params, 1, lvl)

require.NoError(t, evl.InnerSum(ciphertext0, batchSize, n, receiver))

have := make([]uint64, len(want))
require.NoError(t, tc.Ecd.Decode(tc.Dec.DecryptNew(receiver), have))

for i := 0; i < len(want); i += n * batchSize {
require.Equal(t, want[i:i+batchSize], have[i:i+batchSize])
}
})
}
}

for _, lvl := range testLevel {
t.Run(name("Evaluator/InnerSum/N/2 slots", tc, lvl), func(t *testing.T) {
if lvl == 0 {
t.Skip("Skipping: Level = 0")
}
n := 7
batchSize := 13
l := n * batchSize
halfN := tc.Params.MaxSlots() >> 1

galEls := tc.Params.GaloisElementsForInnerSum(batchSize, n)
evl := tc.Evl.WithKey(rlwe.NewMemEvaluationKeySet(nil, tc.Kgen.GenGaloisKeysNew(galEls, tc.Sk)...))

want, _, ciphertext0 := NewTestVector(tc.Params, tc.Ecd, tc.Enc, lvl, tc.Params.NewScale(3))

innersum(want[:halfN], n, batchSize)
innersum(want[halfN:], n, batchSize)

receiver := NewCiphertext(tc.Params, 1, lvl)

require.NoError(t, evl.InnerSum(ciphertext0, batchSize, n, receiver))

have := make([]uint64, len(want))
require.NoError(t, tc.Ecd.Decode(tc.Dec.DecryptNew(receiver), have))

for i, j := 0, halfN; i < halfN; i, j = i+l, j+l {
require.Equal(t, want[i:i+batchSize], have[i:i+batchSize])
require.Equal(t, want[j:j+batchSize], have[j:j+batchSize])
}
})
}
}

func testEvaluatorBfv(tc *TestContext, t *testing.T) {
Expand Down
38 changes: 38 additions & 0 deletions schemes/bgv/evaluator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1505,6 +1505,44 @@ func (eval Evaluator) RotateHoistedLazyNew(level int, rotations []int, op0 *rlwe
return
}

// InnerSum computes the inner sum of the underlying slots (see [rlwe.Evaluator.InnerSum]).
// NB: in the slot encoding of BGV/BFV, the underlying N slots are arranged as 2 rows of N/2 slots.
// If n*batchSize is a multiple of N, InnerSum computes the [rlwe.Evaluator.InnerSum] on the N slots.
// NOTE: In this case, InnerSum performs an addition and a [Evaluator.RotateRowsNew] on top.
// Otherwise, InnerSum computes the [rlwe.Evaluator.InnerSum] of each row separately.
func (eval Evaluator) InnerSum(ctIn *rlwe.Ciphertext, batchSize, n int, opOut *rlwe.Ciphertext) (err error) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall I think it would be clearer to simply add a flag to add the rows together, this would be more natural of the plaintext space. This will also remove the need for a complicated description of how the method behaves.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not for adding a flag as I think most users expect InnerSum to do an inner sum on the whole N slots, without worrying about the internal representation.
For now, the function behaves like that if n * batchSize is a power of 2, which I believe covers most flagship use cases of the function.
If n * batchSize is not a power of 2, the result will not be the inner sum on the whole N slots anyway, regardless of the flag (adding the rows will not give the correct result).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The plaintext algebra is a two dimensional hypercube, so a method on this plaintext algebra would be expected to have two dimensional parameters. Mapping it to a single dimension hides the actual plaintext algebra, requires multiple lines to explain its behavior, and will prevent use cases (for example with this approach it is not possible to aggregate the rows if n*batchsize<N/2).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I understand, but my point is that most users are not aware of the structure of the underlying plaintext space as the issue linked to this PR highlights.
In the current fix, if n*batchSize is a power of 2 (i.e. divides the #of slots), the result is correct whether the user is aware of the 2D structure of the plaintext space and expects an inner sum on each row, or they expect an inner sum on the cleartext vector. This covers most common use cases and the savvy user can easily aggregate rows if they want.

If it is not a power of 2, the result will not be a "textbook" inner sum anyway as the sum on the last batches will wrap around. Then adding the possibility to aggregate the results of these inner sum variants makes this function even more different than what I'd expect an inner sum function to be. This would cover only niche use cases and it seems quite thin to warrant an API change in my opinion.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think users should be made aware of the 2D structure of the plaintext, else we will have more issues about it as it will not behave as how they would expect it in other operations, such as rotations.

Besides this is not an API change since this API is not yet present in the BGV package, so this is the opportunity to make it right.

The goal isn't to cover nich use cases, but to be correct, as this is the API directly interfacing with the scheme. If higher APIs are built on top of it, people are free to do anything they want to abstract the hypercube structure.

Copy link
Contributor Author

@lehugueni lehugueni Nov 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an API change in the sense that legacy code with calls of the form eval.InnerSum(old_parameters) will fail if we add another parameter as there is no method overloading in Go.

Also, I don't see how allowing aggregation makes the inner sum more correct. If anything it makes it more different than a textbook innersum and we don't offer aggregation on other operations (Add, Mul, etc.).

I agree that the correct way would be to let the user do only the inner sum on the rows as before. I just think allowing for the special case n*batchSize=N is fine because it clearly marks the intent of the user, it answers a documented need that must quite common and simplify the life of users.

As a side note, I'm all for adding a Rotate method beside the RotateRows and RotateColumns ones in the long term, so we'd have the basic operations Add, Mul, Rotate that can act on the cleartext vector. But that's another debate :P.

Copy link
Collaborator

@Pro7ech Pro7ech Nov 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with it working for n*batchsize=N, but I disagree that the plaintext algebra should be abstracted already at the scheme level. My experience is that the lowest API should be as flexible as possible, because there is nowhere to fall back to if it happens to not be.

The fact that the bgv.Evaluator already has an InnerSum API through the rlwe.Evaluator is more a result of how Go propagates methods when structs are wrapper together rather than a deliberate choice to make the rlwe.Evaluator.InnerSum available through the bgv.Evaluator. In fact I even think it should not be called InnerSum at the rlwe package level, because this package has no concept of batching.

This is something that we can discuss during the next meeting.

N := eval.parameters.MaxSlots()
l := n * batchSize
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should return an error if n * batchsize > N (or N/2 if instead a flag is used to aggregate the rows, and I don't think the rlwe method does either)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should properly define what are valid parameters for this function (and the underlying one in rlwe).
For instance InnerSum "works" with n*batchSize > N, i.e. it wraps around and keep adding, but it's not optimal performance-wise and I don't see why one would need to do that.

A list of invalid parameters could be:

  • n = 0 or batchSize = 0
  • n*batchSize > N as you suggested
  • N > n*batchSize > N/2 only for bgv.Evaluator.InnerSum

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes indeed, or the domain of valid parameters (if it is shorter to list).


if l%N == 0 {
if n == 1 {
if ctIn != opOut {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method .Copy already performs this check, see

lattigo/core/rlwe/element.go

Line 189 in 3662ccc

func (op *Element[T]) Copy(opCopy *Element[T]) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes indeed, I think this check could be removed in a few other places then.

opOut.Copy(ctIn)
}
return
}

if err = eval.Evaluator.InnerSum(ctIn, batchSize, n/2, opOut); err != nil {
return
}

var ctRot *rlwe.Ciphertext
ctRot, err = eval.RotateRowsNew(opOut)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the rlwe.Evaluator has an internal rlwe.Ciphertext buffer, and also multiple poly buffers, that can be used to have an inplace rotation, instead of having to allocate a new ciphertext.

if err != nil {
return
}

if err = eval.Add(opOut, ctRot, opOut); err != nil {
return
}

return
}

err = eval.Evaluator.InnerSum(ctIn, batchSize, n, opOut)
return
}

// MatchScalesAndLevel updates the both input ciphertexts to ensures that their scale matches.
// To do so it computes t0 * a = opOut * b such that:
// - ct0.Scale * a = opOut.Scale: make the scales match.
Expand Down
2 changes: 1 addition & 1 deletion schemes/bgv/params.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,7 @@ func (p Parameters) GaloisElementForRowRotation() uint64 {
// InnerSum operation with parameters batch and n.
func (p Parameters) GaloisElementsForInnerSum(batch, n int) (galEls []uint64) {
galEls = rlwe.GaloisElementsForInnerSum(p, batch, n)
if n > p.N()>>1 {
if n*batch%p.MaxSlots() == 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if n or batch = 0 then it will add this key, but these are invalid inputs.

galEls = append(galEls, p.GaloisElementForRowRotation())
}
return
Expand Down
Loading