Skip to content

Commit 3848444

Browse files
danmcleranclaude
danmcleran
and
claude
committed
Fix AvgPool1D, AvgPool2D, GlobalAvgPool2D divisor for fixed-point
Same root cause as the dropout scale bug: static_cast<ValueType>(N) for QValue invokes the raw-bits constructor, so dividing by PoolSize/ WindowArea/Area was actually dividing by N/2^FracBits. Existing fixed-point pool tests didn't exist, so this had been silently producing wrong averages in any fixed-point pipeline that used AvgPool*. Construct the divisor via the same SFINAE-dispatched pattern as Dropout::scale: cast for floating-point, (FixedPart, FractionalPart) constructor for QValue. MaxPool* paths are unaffected — they don't divide. Adds three pinning tests: - AvgPool1D Q8.8 with whole-number averages (3.0, 7.0) - AvgPool2D Q8.8 with 2x2 windows averaging to 2/4/6/8 - GlobalAvgPool2D Q8.8 reducing 3x3x2 to 1.0/3.0 per channel Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 01075e1 commit 3848444

3 files changed

Lines changed: 118 additions & 7 deletions

File tree

‎cpp/pool1d.hpp‎

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
#pragma once
2424

2525
#include <cstddef>
26+
#include <type_traits>
2627

2728
namespace tinymind {
2829
/**
@@ -182,7 +183,7 @@ namespace tinymind {
182183
sum += input[inputStart + k];
183184
}
184185

185-
output[outputOffset + pos] = sum / static_cast<ValueType>(PoolSize);
186+
output[outputOffset + pos] = sum / divisor();
186187
}
187188
}
188189
}
@@ -208,7 +209,7 @@ namespace tinymind {
208209
for (size_t pos = 0; pos < OutputLength; ++pos)
209210
{
210211
const size_t inputStart = inputOffset + pos * Stride;
211-
const ValueType grad = outputDeltas[outputOffset + pos] / static_cast<ValueType>(PoolSize);
212+
const ValueType grad = outputDeltas[outputOffset + pos] / divisor();
212213

213214
for (size_t k = 0; k < PoolSize; ++k)
214215
{
@@ -219,6 +220,23 @@ namespace tinymind {
219220
}
220221

221222
private:
223+
// Build the PoolSize divisor as ValueType. For floating-point a cast
224+
// is enough; for QValue the (FixedPart, FractionalPart) constructor
225+
// is required because QValue(int) treats its argument as raw bits.
226+
template<typename T = ValueType>
227+
static typename std::enable_if<std::is_floating_point<T>::value, T>::type
228+
divisor()
229+
{
230+
return static_cast<T>(PoolSize);
231+
}
232+
233+
template<typename T = ValueType>
234+
static typename std::enable_if<!std::is_floating_point<T>::value, T>::type
235+
divisor()
236+
{
237+
return T(static_cast<typename T::FixedPartFieldType>(PoolSize), 0u);
238+
}
239+
222240
static_assert(InputLength >= PoolSize, "Input length must be >= pool size.");
223241
static_assert(Stride > 0, "Stride must be > 0.");
224242
static_assert(PoolSize > 0, "Pool size must be > 0.");

‎cpp/pool2d.hpp‎

Lines changed: 37 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
#pragma once
2424

2525
#include <cstddef>
26+
#include <type_traits>
2627

2728
namespace tinymind {
2829
/**
@@ -168,7 +169,7 @@ namespace tinymind {
168169
sum += input[inIdx];
169170
}
170171
}
171-
output[outPixelOffset + c] = sum / static_cast<ValueType>(WindowArea);
172+
output[outPixelOffset + c] = sum / divisor();
172173
}
173174
}
174175
}
@@ -191,8 +192,7 @@ namespace tinymind {
191192

192193
for (size_t c = 0; c < Channels; ++c)
193194
{
194-
const ValueType grad = outputDeltas[outPixelOffset + c]
195-
/ static_cast<ValueType>(WindowArea);
195+
const ValueType grad = outputDeltas[outPixelOffset + c] / divisor();
196196
for (size_t kh = 0; kh < PoolH; ++kh)
197197
{
198198
for (size_t kw = 0; kw < PoolW; ++kw)
@@ -207,6 +207,23 @@ namespace tinymind {
207207
}
208208

209209
private:
210+
// Build the WindowArea divisor as ValueType. Cast for floating-point;
211+
// (FixedPart, FractionalPart) constructor for QValue, since QValue(int)
212+
// would treat its argument as raw bits instead of a value.
213+
template<typename T = ValueType>
214+
static typename std::enable_if<std::is_floating_point<T>::value, T>::type
215+
divisor()
216+
{
217+
return static_cast<T>(WindowArea);
218+
}
219+
220+
template<typename T = ValueType>
221+
static typename std::enable_if<!std::is_floating_point<T>::value, T>::type
222+
divisor()
223+
{
224+
return T(static_cast<typename T::FixedPartFieldType>(WindowArea), 0u);
225+
}
226+
210227
static_assert(H >= PoolH, "Input height must be >= pool height.");
211228
static_assert(W >= PoolW, "Input width must be >= pool width.");
212229
static_assert(StrideH > 0 && StrideW > 0, "Strides must be > 0.");
@@ -253,15 +270,15 @@ namespace tinymind {
253270

254271
for (size_t c = 0; c < Channels; ++c)
255272
{
256-
output[c] = output[c] / static_cast<ValueType>(Area);
273+
output[c] = output[c] / divisor();
257274
}
258275
}
259276

260277
void backward(ValueType const* const outputDeltas, ValueType* inputDeltas) const
261278
{
262279
for (size_t c = 0; c < Channels; ++c)
263280
{
264-
const ValueType grad = outputDeltas[c] / static_cast<ValueType>(Area);
281+
const ValueType grad = outputDeltas[c] / divisor();
265282
for (size_t h = 0; h < H; ++h)
266283
{
267284
for (size_t w = 0; w < W; ++w)
@@ -273,6 +290,21 @@ namespace tinymind {
273290
}
274291

275292
private:
293+
// See pool1d.hpp / pool2d.hpp::AvgPool2D::divisor for rationale.
294+
template<typename T = ValueType>
295+
static typename std::enable_if<std::is_floating_point<T>::value, T>::type
296+
divisor()
297+
{
298+
return static_cast<T>(Area);
299+
}
300+
301+
template<typename T = ValueType>
302+
static typename std::enable_if<!std::is_floating_point<T>::value, T>::type
303+
divisor()
304+
{
305+
return T(static_cast<typename T::FixedPartFieldType>(Area), 0u);
306+
}
307+
276308
static_assert(H > 0 && W > 0, "Spatial dimensions must be > 0.");
277309
static_assert(Channels > 0, "Channels must be > 0.");
278310
};

‎unit_test/nn/nn_unit_test.cpp‎

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4431,6 +4431,22 @@ BOOST_AUTO_TEST_CASE(test_case_avgpool1d_pool3)
44314431
BOOST_TEST(fabs(output[1] - 15.0) < 0.001);
44324432
}
44334433

4434+
BOOST_AUTO_TEST_CASE(test_case_avgpool1d_fixed_point)
4435+
{
4436+
// Pin down the fixed-point divisor: average of [2.0, 4.0] in Q8.8 must be
4437+
// exactly 3.0, not raw=PoolSize.
4438+
typedef tinymind::QValue<8, 8, true, tinymind::RoundUpPolicy> ValueType;
4439+
tinymind::AvgPool1D<ValueType, 4, 2, 2, 1> pool;
4440+
4441+
ValueType input[4] = {ValueType(2, 0), ValueType(4, 0), ValueType(6, 0), ValueType(8, 0)};
4442+
ValueType output[2];
4443+
pool.forward(input, output);
4444+
4445+
// (2+4)/2 = 3.0; (6+8)/2 = 7.0.
4446+
BOOST_TEST(output[0].getValue() == ValueType(3, 0).getValue());
4447+
BOOST_TEST(output[1].getValue() == ValueType(7, 0).getValue());
4448+
}
4449+
44344450
// ============================================================
44354451
// Dropout tests
44364452
// ============================================================
@@ -6743,6 +6759,51 @@ BOOST_AUTO_TEST_CASE(test_case_globalavgpool2d_forward)
67436759
BOOST_TEST(std::fabs(output[1] - 3.0) < 1e-9);
67446760
}
67456761

6762+
BOOST_AUTO_TEST_CASE(test_case_avgpool2d_fixed_point)
6763+
{
6764+
// 4x4 input, 2x2 window, stride 2 -> 2x2 output. Divisor must be 4.0
6765+
// (a Q-format value), not raw=4.
6766+
typedef tinymind::QValue<8, 8, true, tinymind::RoundUpPolicy> ValueType;
6767+
tinymind::AvgPool2D<ValueType, 4, 4, 1, 2, 2, 2, 2> pool;
6768+
6769+
ValueType input[16];
6770+
// Row-major NHWC, channel 0. Use values that average to whole numbers.
6771+
const int values[16] = {
6772+
2, 2, 4, 4,
6773+
2, 2, 4, 4,
6774+
6, 6, 8, 8,
6775+
6, 6, 8, 8
6776+
};
6777+
for (size_t i = 0; i < 16; ++i) input[i] = ValueType(values[i], 0);
6778+
6779+
ValueType output[4];
6780+
pool.forward(input, output);
6781+
6782+
BOOST_TEST(output[0].getValue() == ValueType(2, 0).getValue());
6783+
BOOST_TEST(output[1].getValue() == ValueType(4, 0).getValue());
6784+
BOOST_TEST(output[2].getValue() == ValueType(6, 0).getValue());
6785+
BOOST_TEST(output[3].getValue() == ValueType(8, 0).getValue());
6786+
}
6787+
6788+
BOOST_AUTO_TEST_CASE(test_case_globalavgpool2d_fixed_point)
6789+
{
6790+
// 3x3x2 input, all ones / threes per channel; GAP must produce exactly 1.0 / 3.0.
6791+
typedef tinymind::QValue<8, 8, true, tinymind::RoundUpPolicy> ValueType;
6792+
tinymind::GlobalAvgPool2D<ValueType, 3, 3, 2> gap;
6793+
6794+
ValueType input[18];
6795+
for (size_t i = 0; i < 9; ++i)
6796+
{
6797+
input[i * 2 + 0] = ValueType(1, 0);
6798+
input[i * 2 + 1] = ValueType(3, 0);
6799+
}
6800+
ValueType output[2];
6801+
gap.forward(input, output);
6802+
6803+
BOOST_TEST(output[0].getValue() == ValueType(1, 0).getValue());
6804+
BOOST_TEST(output[1].getValue() == ValueType(3, 0).getValue());
6805+
}
6806+
67466807
// ============================================================
67476808
// Benchmark harness tests
67486809
// ============================================================

0 commit comments

Comments
 (0)