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.

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)

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

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

The method .Copy already performs this check, see

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

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.

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).

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).

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.

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.

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.

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.

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