Repository navigation
Audit layers.py and cnn.py: fix BatchNorm eps and inference leak, add input validation and gradient guards - #8
Merged
Merged
Conversation
…nd pooling windows
An audit of layers.py and cnn.py. The gradients themselves turned out to be
correct -- BatchNorm's dE/dX and dE/dgamma match finite differences to ~1e-9, and
Conv2D's holds for padding 0/1/2 and stride 1/2 -- but four defects surround them.
BatchNormalization's eps was hardcoded at 0.01, a thousand times the
conventional 1e-5, and it sits INSIDE the square root: the applied scale is
1/sqrt(var + eps). Once eps is comparable to a feature's variance it dominates
and the feature is left unnormalized, which is the layer's entire purpose:
feature variance under-scaled by
1.0 0.5%
0.01 29.3%
0.0001 90.0%
Measured end to end, a feature with standard deviation 0.01 came out of the layer
at 0.095 rather than ~1. eps is now 1e-5 and configurable, so it can still be
raised for very small batches where the variance estimate itself is noisy.
BatchNormalization also initialised its running statistics on ANY first forward
pass, so calling forward(training=False) on an untrained layer seeded them from
the inference batch and then normalized that batch by its own mean and variance
-- exactly the leakage running estimates exist to prevent, and silently: two test
samples came back as -0.995 and +0.995 as though centred on themselves. That path
now raises with an explanation. Training first, then inferring, is unchanged.
Dense.set_weights accepted any shapes, so a mismatch surfaced later as a
broadcasting ValueError from inside forward, far from the assignment responsible.
Both arguments are now checked against the layer's own dimensions.
Pooling2D raised a bare Exception("Invalid output dimension!") when the window
and stride did not tile the input, naming none of the three values at fault. It
now reports the window, the stride, the input size and the fractional output size
it would have produced.
… striding
BatchNorm's backward pass was only shape-tested, despite being the most involved
expression in the module: the batch mean and variance each depend on every
sample, so a sample's gradient flows back through those shared statistics too.
It now has a central-difference check, plus one for dE/dgamma.
The incoming error in that check is RANDOM rather than all-ones, and that detail
is load-bearing. The variance-path term is
X_centered * stddev_inv**2 * sum(output_error * X_centered)
and X_centered sums to zero by construction, so under a constant error the whole
sum is zero and the term vanishes. The first version of this test used
np.ones_like and consequently passed with that term deleted from the
implementation -- the mutation run is what exposed it. A companion test asserts
the constant-error and random-error gradients actually differ, so the check
cannot silently degenerate back to the weaker form.
Also added: BatchNorm now verified to normalize a low-variance feature, to refuse
inference before training, and to keep eps configurable; Dense.set_weights shape
validation; pooling window/stride validation; and a Conv2D gradient check across
padding 0/1/2 and stride 1/2, since the existing check only covered padding=0
stride=1 and both change the im2col index arithmetic.
Two more findings from the same audit. Reshape.backward before a forward pass returned a WRONGLY-SHAPED gradient in silence. prev_shape starts as None, and arr.reshape(None) does not raise -- it returns the array with its existing shape -- so the previous layer received the wrong thing with no error. Its siblings (Dropout, Flatten, pooling) at least raised an AttributeError. Not reachable through the NN container, which always runs forward first, but the failure mode was the quiet kind. Conv2D took no input validation at all: - stride=0 reached the output-size formula and raised a bare ZeroDivisionError. stride is now required to be an integer >= 1, and input_shape to be a 3-tuple. - A channel-count mismatch surfaced deep inside the matmul as "matmul: Input operand 1 has a mismatch in its core dimension", which mentions nothing about channels. forward now says which count the kernels were built for and which it received, and rejects a non-batched (3-D) input. The declared SPATIAL dimensions of input_shape turn out to be unused: only input_shape[2] is read, and the convolution derives its output size from the input it is actually handed, so a layer declared (5, 5, 2) processes a 7x7x2 input correctly. That is documented as advisory rather than enforced, since the behaviour is right and changing it would break working code -- with a test pinning it so the decision is explicit rather than accidental.
activation.py sat at 91% because none of the ten prime2 methods were tested. prime2 is unused by the library's own training loop -- first-order optimizers only need prime -- which is exactly why it could drift unnoticed. All ten were verified correct against a finite difference of prime before being pinned, so this adds no fixes, only the guard that was missing. Also covered: the piecewise-linear activations having exactly zero curvature away from their kinks, Exponential being its own second derivative, softplus' second derivative being sigma(z)(1 - sigma(z)), and SoftMax deliberately having no prime2 (it is not element-wise and defines only its Jacobian diagonal). Corrupting SoftPlus.prime2 or ELU.prime2 now fails these tests. activation.py coverage 91% -> 98%.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
An audit of
layers.pyandcnn.py, the last unaudited code in the library.Every commit is independently green and lint-clean.
First, what is not broken
The gradients are correct, and that is worth stating because it was the main thing the audit set out to check:
dE/dXanddE/dgammamatch finite differences to ~1e-9.dE/dXholds for padding 0/1/2 and stride 1/2 — combinations the existing check never covered, and both change the im2col index arithmetic.prime2second derivatives match a finite difference ofprime.No fixes were needed in any of that. What follows is the six defects surrounding it.
BatchNormalization.epswas 1000x too largeIt was hardcoded at 0.01 against a conventional 1e-5, and it sits inside the square root — the applied scale is
1/sqrt(var + eps). Once eps is comparable to a feature's variance it dominates, and the layer stops normalising the features that most need it:Measured end to end, a feature with standard deviation 0.01 left the layer at 0.095 instead of ~1 — a normalisation layer not normalising. Now 1e-5 and configurable, so it can still be raised for very small batches where the variance estimate is itself noisy.
I checked the obvious risk of lowering it: at batch size 1, and for a constant feature,
X_centeredis exactly zero, so the output is exactlybetaregardless of eps. No amplification in practice.This is a numerical behaviour change — networks using BatchNorm will produce slightly different values than before.
BatchNorm leaked test statistics at inference
Running statistics were initialised on any first forward pass, so
forward(training=False)on an untrained layer seeded them from the inference batch and then normalised that batch by its own mean and variance — precisely the leakage running estimates exist to prevent, and silently: two test samples came back as-0.995and+0.995, as though centred on themselves. That path now raises with an explanation. Train-then-infer is unchanged.Input validation that was missing entirely
Reshape.backwardbeforeforwardarr.reshape(None)does not raise)AssertionErrorConv2D(stride=0)ZeroDivisionErrorfrom the output-size formulaValueErrorConv2Dchannel mismatchmatmul: Input operand 1 has a mismatch in its core dimensionConv2Dnon-batched inputValueErrorDense.set_weightswrong shapesforwardValueErrornaming the expected shapesPooling2Dwindow that doesn't tileException("Invalid output dimension!")Conv2D's declared spatialinput_shapeturns out to be unused — onlyinput_shape[2]is read, and the convolution derives its output size from the input it is actually given, so a layer declared(5, 5, 2)processes a7x7x2input correctly. That is now documented as advisory rather than enforced, since the behaviour is right and enforcing it would break working code. A test pins it so the decision is explicit rather than accidental.A flaw in my own test, caught by mutation
Worth flagging plainly. My first BatchNorm gradient check used an all-ones incoming error, and it passed with the variance-path term deleted from the implementation. That term is
and
X_centeredsums to zero by construction, so under a constant error the whole sum is zero and the term vanishes — the test looked rigorous while exercising nothing. Only the mutation run revealed it.Fixed by using a random error, plus a companion test asserting the constant-error and random-error gradients genuinely differ, so the check cannot silently degenerate back to the weaker form. Both BatchNorm gradient terms now fail when removed.
Verification
Ten mutations, each reintroducing one defect; all are caught:
epsback to 0.01Dense.set_weightsvalidation removedPooling2Derror back to bareExceptionReshape.backwardguard removedConv2Dstride validation removedConv2Dchannel check removedConv2Dndim check removedSoftPlus.prime2corruptedELU.prime2corruptedCoverage:
activation.py91% → 98%,cnn.py92% → 94%,layers.py90% → 92%.Still unaudited after this
Two areas remain, deliberately left for a separate pass rather than growing this branch:
si/unsupervised/(kmeans.py,pca.py—KMeansuses an unseeded RNG) andsi/supervised/knn.py,linreg.py,logreg.py(high coverage, never read for bugs;linreg.pyis the lowest at 84%).