Fix inner sum in BGV #513
Fix inner sum in BGV #513
Conversation
| } | ||
|
|
||
| var ctRot *rlwe.Ciphertext | ||
| ctRot, err = eval.RotateRowsNew(opOut) |
There was a problem hiding this comment.
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.
| // 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) { | ||
| N := eval.parameters.MaxSlots() | ||
| l := n * batchSize |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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 = 0orbatchSize = 0n*batchSize > Nas you suggestedN > n*batchSize > N/2only forbgv.Evaluator.InnerSum
There was a problem hiding this comment.
Yes indeed, or the domain of valid parameters (if it is shorter to list).
|
|
||
| if l%N == 0 { | ||
| if n == 1 { | ||
| if ctIn != opOut { |
There was a problem hiding this comment.
The method .Copy already performs this check, see
Line 189 in 3662ccc
There was a problem hiding this comment.
Ah yes indeed, I think this check could be removed in a few other places then.
| // 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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @@ -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 { | |||
There was a problem hiding this comment.
if n or batch = 0 then it will add this key, but these are invalid inputs.
Fix #510 and add corresponding tests.
Issue
bgv.Evaluator.InnerSumwas not working properly whenn*batchSize = N(whereNis the number of slots) asInnerSumwas acting separately on the "rows" of the underlying plaintext.New behaviour
n*batchSizeis a multiple ofN, the inner sum is computed over the fullNslots via a rotation of the rows and an addition as showcased in Bug/Regression [BGV]: Wrong result from innerSum for params.N() elements #510. (Note: calling the inner sum withn*batchSize = k*Nfork > 1does not make sense in terms of performance but we support it).bgv.Evaluator.InnerSumacts as before (i.e. the inner sum is computed on both rows "independently").