|
| 1 | +# P12 — `brainpy/dnn` expert review (2026-06-19) |
| 2 | + |
| 3 | +Branch: `fix/audit-20260619-dnn`. Scope: `brainpy/dnn/{activations,base,conv,dropout,function,interoperation_flax,linear,normalization,pooling}.py` + co-located tests. |
| 4 | + |
| 5 | +Environment: jax 0.10.2, brainstate 0.5.1, braintools 0.1.10, brainunit 0.5.1 (CPU). |
| 6 | + |
| 7 | +Severity legend: Critical (silently wrong/crash in default usage), High (wrong in realistic cases / broken public API), Medium (edge/fragility/error-handling), Low (style/docs — record only). |
| 8 | + |
| 9 | +--- |
| 10 | + |
| 11 | +### P12-M1 — `LayerNorm` shape-mismatch error does `", ".join(<ints>)` → masks real error with `TypeError` [Medium] |
| 12 | +- File: brainpy/dnn/normalization.py:536 |
| 13 | +- Category: edge/error |
| 14 | +- What: When the trailing input dims do not match `normalized_shape`, the guard raises |
| 15 | + `ValueError(f'... (..., {", ".join(self.normalized_shape)}), but we got {x.shape}')`. |
| 16 | + `self.normalized_shape` is a tuple of `int`, so `", ".join(...)` raises |
| 17 | + `TypeError: sequence item 0: expected str instance, int found` instead of the intended `ValueError`. |
| 18 | +- Why it's a bug: The user-facing diagnostic is replaced by an opaque `TypeError`, hiding the actual |
| 19 | + shape problem. Any wrong-shape input to an affine/non-affine `LayerNorm` hits this. |
| 20 | +- Repro: |
| 21 | + ```python |
| 22 | + bp.dnn.LayerNorm(10)(bm.random.randn(2, 5, 8)) # -> TypeError, not ValueError |
| 23 | + ``` |
| 24 | +- Fix: `", ".join(map(str, self.normalized_shape))` (and format the expected shape readably). |
| 25 | +- Tests: `normalization_test.py::Test_Normalization::test_LayerNorm_shape_mismatch_raises_valueerror` |
| 26 | +- Status: fixed |
| 27 | +- (== prior audit M-26) |
| 28 | + |
| 29 | +--- |
| 30 | + |
| 31 | +### P12-M2 — Pooling rejects the leftmost negative `channel_axis` (`channel_axis == -x_dim`) [Medium] |
| 32 | +- File: brainpy/dnn/pooling.py:118 (`Pool._infer_shape`), 390 (`_MaxPoolNd._infer_shape`), 787 (`AdaptivePool.update`) |
| 33 | +- Category: edge/error |
| 34 | +- What: The bound check is `if channel_axis and not 0 <= abs(channel_axis) < x_dim: raise`. |
| 35 | + Using `abs(channel_axis)` makes `channel_axis == -x_dim` (the valid leftmost axis, e.g. `-3` for a |
| 36 | + 3-D `(C, H, W)` input) fail the check and raise `ValueError`. |
| 37 | +- Why it's a bug: A legitimate, common channels-first layout (`channel_axis=-3` on `(C,H,W)`, or |
| 38 | + `-4` on `(N,C,H,W)`) is wrongly rejected. The correct numpy-style bound is `-x_dim <= axis < x_dim`. |
| 39 | +- Repro: |
| 40 | + ```python |
| 41 | + bp.dnn.MaxPool2d(2, channel_axis=-3)(bm.random.randn(6, 4, 4)) # ValueError: Invalid channel axis -3 |
| 42 | + ``` |
| 43 | +- Fix: Replace the `abs()` bound test with `not -x_dim <= channel_axis < x_dim` in all three sites. |
| 44 | +- Tests: `pooling_layers_test.py::TestPoolingChannelAxis::test_maxpool2d_leftmost_negative_channel_axis`, |
| 45 | + `...::test_adaptiveavgpool2d_leftmost_negative_channel_axis` |
| 46 | +- Status: fixed |
| 47 | +- (== prior audit M-27) |
| 48 | + |
| 49 | +--- |
| 50 | + |
| 51 | +### P12-M3 — `BatchNorm` stores the *biased* batch variance into `running_var` (PyTorch uses unbiased) [Medium] |
| 52 | +- File: brainpy/dnn/normalization.py:172-174 |
| 53 | +- Category: numerics |
| 54 | +- What: In fit mode the running estimate is updated with the biased population variance |
| 55 | + `var = mean_of_square - mean**2` (divisor `N`). PyTorch / the conventional BatchNorm running statistic |
| 56 | + uses the *unbiased* sample variance (divisor `N-1`, i.e. Bessel's correction) for the running buffer, |
| 57 | + while keeping the biased variance only for normalizing the current batch. |
| 58 | +- Why it's a bug: The running variance used at eval time is systematically too small by a factor of |
| 59 | + `(N-1)/N`. For small batch/window counts `N` this is a meaningful (a few percent) bias in the |
| 60 | + eval-time normalization — i.e. inference results drift from the trained reference. |
| 61 | +- Repro: |
| 62 | + ```python |
| 63 | + bn = bp.dnn.BatchNorm1d(3, affine=False); bp.share.save(fit=True) |
| 64 | + bn(bm.random.randn(4, 5, 3)) # N = 20 |
| 65 | + # running_var == 0.99*1 + 0.01*biased_var, not unbiased_var |
| 66 | + ``` |
| 67 | +- Fix: Scale the variance fed into the `running_var` EMA by `N/(N-1)` (with `N` = number of reduced |
| 68 | + elements), guarding `N == 1`. The batch normalization itself keeps the biased `var`. |
| 69 | +- Tests: `normalization_test.py::Test_Normalization::test_BatchNorm_running_var_is_unbiased` |
| 70 | +- Status: fixed |
| 71 | +- (== prior audit M-25) |
| 72 | + |
| 73 | +--- |
| 74 | + |
| 75 | +### P12-L1 — `Flatten` default `start_dim=0` contradicts its docstring example and PyTorch (`start_dim=1`) [Low] |
| 76 | +- File: brainpy/dnn/function.py:91 (default), 74-87 (docstring example) |
| 77 | +- Category: api-drift/style |
| 78 | +- What: `Flatten.__init__` defaults `start_dim=0`. The class docstring example claims |
| 79 | + `Flatten()` on `(32, 1, 5, 5)` yields `(32, 25)` (PyTorch's `start_dim=1` semantics), but in |
| 80 | + `NonBatchingMode` the actual default flattens the batch dim too → `(800,)`. |
| 81 | +- Why it's a bug: The documented contract and the implemented default disagree. PyTorch's `nn.Flatten` |
| 82 | + default is `start_dim=1`. |
| 83 | +- Repro: |
| 84 | + ```python |
| 85 | + bp.dnn.Flatten()(bm.random.randn(32, 1, 5, 5)).shape # (800,), docstring says (32, 25) |
| 86 | + ``` |
| 87 | +- Fix: recorded only. NOTE: changing the default to `1` would change the documented `NonBatchingMode` |
| 88 | + contract and break `function_test.py::test_flatten_non_batching_mode` (asserts `(600,)` from default |
| 89 | + start_dim under `NonBatchingMode`). The discrepancy is documentation/default-value drift, not a |
| 90 | + silent numeric error, and a default change is a cross-cutting API change. Left for maintainers to |
| 91 | + decide (fix docs vs. change default + migrate the test). |
| 92 | +- Tests: none |
| 93 | +- Status: recorded-only |
| 94 | + |
| 95 | +--- |
| 96 | + |
| 97 | +## Cross-check vs `dev/issues-found-20260618.md` (dnn entries) |
| 98 | + |
| 99 | +- **C-05** (`GroupNorm`/`InstanceNorm` reduce over the group axis): **already fixed** in this tree |
| 100 | + (`normalization.py:640` reduces over spatial + within-group channel axis, keeps the group axis). |
| 101 | + Verified: `GroupNorm(3,6) != GroupNorm(1,6) != GroupNorm(6,6)`, per-group means ≈ 0. No action. |
| 102 | +- **H-51** (`BatchNorm`/affine `LayerNorm`/`GroupNorm` crash out-of-the-box under default mode): |
| 103 | + **already fixed** (`BatchNorm` defaults to `training_mode`; affine params wrapped as `Variable` vs |
| 104 | + `TrainVar` per mode instead of a hard assert). No action. |
| 105 | +- **M-25**: still present → fixed here as **P12-M3**. |
| 106 | +- **M-26**: still present → fixed here as **P12-M1**. |
| 107 | +- **M-27**: still present → fixed here as **P12-M2**. |
| 108 | +- **M-28** (`Flatten` default): still present → recorded as **P12-L1** (see note; default change out of safe scope). |
| 109 | + |
| 110 | +## Checked and found correct (no action) |
| 111 | +- `Dropout`: `prob` = keep-probability; `bernoulli(prob)` keeps with prob `prob`; survivors scaled by |
| 112 | + `1/prob == 1/(1-rate)`; eval (`fit=False`) is a no-op. Correct. |
| 113 | +- `Conv*` / `ConvTranspose*`: kernel shapes, `feature_group_count=groups`, dimension numbers, |
| 114 | + bias broadcast (channels-last), non-batching unsqueeze/squeeze, `SAME`/`VALID`/int/tuple padding |
| 115 | + normalization. Correct (smoke-checked shapes & a transpose upsample). |
| 116 | +- `AvgPool`/`_AvgPoolNd` non-VALID averaging via a second `reduce_window` count. Correct. |
| 117 | +- `GroupNorm` affine broadcast via `lax.broadcast_to_rank` to channels-last. Correct. |
| 118 | +- Activation wrappers delegate to `bm.*`; formulas match docstrings. `Softmax2d` uses axis `-3` |
| 119 | + (channels-first `(N,C,H,W)`/`(C,H,W)`), matching its documented contract. |
| 120 | +- `interoperation_flax`: Flax round-trip param flatten/unflatten, `ToFlaxRNNCell` carry handling. |
| 121 | + Correct for flax present/absent. |
0 commit comments