Generic Implementation of Global Average Pooling Fails for Integer Types

In the file: aidge_backend_cpu/include/aidge/backend/cpu/operator/GlobalAveragePoolingImpl_kernels.hpp

The current implementation of the GlobalAveragePoolingImpl_cpu_forward_kernel function does not correctly handle integer types. The issue arises because the function uses fmaf to compute the mean, which is not suitable for integer types. This leads to incorrect results when the input tensor contains integer values.

The function assumes that the input type I and output type O can be cast to and from floating-point numbers, which is not valid for integer types. Specifically, the line

mean = fmaf(filter_start[i] - mean, 1.0f/(i+1), mean);

uses fmaf, which operates on float values. When I is an integer type, this results in incorrect calculations due to improper type handling.

I suggest to handle integer types separately by accumulating the sum of integer values and then performing integer division to compute the mean

added Bug 🐛 label

changed the description

mentioned in merge request aidge_quantization!35 (closed)

mentioned in commit 3754ec3f

mentioned in merge request !133 (merged)

mentioned in commit 114cba02

@noamzerah Please check !133 (merged). The issue was that the mean variable type should be floating point in all cases.

Note that with current behavior, if the output is integer, the mean is truncated to integer, not rounded. Should we change this behavior?

Yes, I think it would be better to round the values to improve accuracy and avoid the bias introduced by truncation.

Ok, done. Could you check if the MR is working for you?

It turns out that I’m losing 0.2% accuracy on resnet18 with this fix. I’ll investigate to find the cause.

In the version I wrote yesterday, I computed the average by accumulating the sum in an int64 and then performing an integer division. This approach helps avoid rounding errors, which I believe is why it produced better results on ResNet.

The issue with just accumulating the sum is that it is not numerically stable. It can lead to overflow...

It's true that in this case, we don't have many alternatives. Since the loss remains minimal and unavoidable, I believe we can proceed with the merge request.

You may try replace float to double in:

template <typename T>
typename std::enable_if<!std::is_floating_point<T>::value, T>::type
stableMean(const T* vec, size_t size) {
  float mean = 0;
  for (size_t i = 0; i < size; ++i) {
    mean = std::fma<float>(vec[i] - mean, 1.0f / (i + 1), mean);
  }
  return mean;
}

See if this makes any difference. Otherwise, it might be that the score is improved just by chance, despite being more accurate...

This allows me to gain 0.1% on ResNet18 so, we're almost at the same accuracy as when accumulating in an int64.

It might be that the rounding is different? Did you do a truncation or rounding?

I used a truncation

Ok, it might be that the truncation actually performs better in this case, you can try removing std::nearbyint to check.

I've already done that, I'm waiting for the result.

I get the same results whether using rounding or truncation.

Alright, do we keep the MR as is? Can you validate if if it is OK?

Yes, we can keep the MR as it is. I don't think we can maximize the gain any further

mentioned in commit 2bb9f35b

closed

mentioned in commit a424079c

mentioned in commit maab05/aidge_backend_cpu@2406fba1