Fix DeepSVDD not training due to disabled backward pass - #704
Conversation
The DeepSVDD training loop had loss.backward() commented out, so no gradients were ever computed and optimizer.step() was a no-op: the model never learned and the anomaly scores reflected the randomly initialised network (issue yzhao062#606). Simply uncommenting loss.backward() raises "Trying to backward through the graph a second time", because the w_d L2 penalty term was built once before the epoch loop and then reused in every batch's loss, so the second backward re-traverses its freed graph. Recomputing w_d inside the batch loop makes it part of each batch's own graph and resolves this. Also move the best-model bookkeeping out of the batch loop: epoch_loss is accumulated over the whole epoch, so comparing it to best_loss after every batch saved an early-in-epoch state rather than the best epoch (issue yzhao062#641). Verified on synthetic data: training loss now decreases smoothly (4.53 -> ~0 over 40 epochs) instead of staying flat (~8.8), the existing test_deepsvdd.py suite passes, and ROC AUC is unaffected. Note that best_model_dict is still not loaded back into model_ by decision_function; that is left unchanged here. Fixes yzhao062#606. Fixes yzhao062#641.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 80e9ed85f5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| loss = torch.mean(dist) + w_d | ||
|
|
||
| # loss.backward() | ||
| loss.backward() |
There was a problem hiding this comment.
When optimizer='sparseadam' is selected from the public optimizer map, this newly enabled backward pass populates dense gradients for the nn.Linear-only network, and torch.optim.SparseAdam.step() rejects dense gradients at runtime. Please either remove/validate this optimizer choice for DeepSVDD or route it to an optimizer that supports dense parameters, otherwise that advertised option now crashes during fit().
Useful? React with 👍 / 👎.
| loss = torch.mean(dist) + w_d | ||
|
|
||
| # loss.backward() | ||
| loss.backward() |
There was a problem hiding this comment.
With the newly enabled backward pass, the default c=None path now actually optimizes against self.c, but that attribute is still the scalar 0.0 because _init_c() stores the initialized center on self.model_.c instead. For ordinary default fits this trains the linear network toward an all-zero mapping rather than the initialized SVDD center, so longer training can collapse anomaly scores instead of learning the hypersphere. Please assign/detach the initialized center onto the detector before backpropagating.
Useful? React with 👍 / 👎.
| # the batch loop, not inside it) | ||
| if epoch_loss < best_loss: | ||
| best_loss = epoch_loss | ||
| best_model_dict = self.model_.state_dict() |
There was a problem hiding this comment.
When a later epoch is worse than the one recorded here, self.model_.state_dict() does not freeze the current tensors; it returns references to the module parameters, and subsequent optimizer.step() calls mutate the tensors held by best_model_dict. As a result, in any non-monotonic training run the advertised best-model bookkeeping still ends up holding the final weights rather than the best epoch. Please deep-copy or clone the state dict at the point it is selected.
Useful? React with 👍 / 👎.
state_dict() returns references to the live parameters, so the following epoch's optimizer.step() mutated the dict that was meant to record the best epoch. Deep-copy it so best_model_dict holds the weights it claims to.
|
Thanks — I worked through all three of the automated suggestions. Two are real, one is real but pre-existing and unrelated, and the third turned out to be more serious than described. Details, since the last one affects how you may want to sequence this PR. 3. Cloning the best epoch's state dict — real, fixed in 3a5606b. Confirmed: 2. The stated mechanism isn't what happens. It never reaches
1. The centre — real, and worth pausing on. Confirmed: That matters much more now that the backward pass actually runs. With
At 100 epochs every embedding is identically zero, so every anomaly score is zero and the detector is degenerate. My earlier "loss → ~0" validation note was, in hindsight, measuring the collapse rather than convergence — apologies, I should have caught that. I tried the obvious fix (
I should also be straight about the ceiling: with the centre wired up correctly I measured mean AUC ≈ 0.50 across 5 seeds at the default 100 epochs (range 0.20–0.77), essentially unchanged whether the centre is taken pre- or post-activation. So the centre alone does not make this a good detector — there is more wrong here than one PR can responsibly fix. Given that, how would you like to proceed?
I'd lean towards the first, with the follow-up opened immediately, but I'm happy either way. Either way it seems worth tracking the collapse separately, since it is a real consequence of turning training on. |
#704 restored the commented-out loss.backward(), which exposed three pre-existing defects it had been masking: - fit() set self.c = 0.0 while _init_c() wrote the computed center to the inner module, so the objective never saw it. For a bias-free ReLU network the all-zero weights map every input to 0, making c=0 exactly the trivial solution of Ruff et al., ICML 2018, Proposition 1. On a 17-dataset ODDS benchmark the merged state returned a single distinct score on 10 of 17 datasets while still clearing a ROC floor via 1e-24 floating-point noise. - The optimizer was built with no learning rate (Adam default 1e-3) and passed l2_regularizer=0.1 as weight_decay, five orders of magnitude above the reference 5e-7; that decay drives weights toward zero and compounds the collapse. - best_model_dict was stored but never loaded back, so scoring used final-epoch weights. fit() also wrote the fitted center into the constructor parameter c, which leaked fitted state through get_params(), made clone() start pre-seeded, and made a refit train a newly built network against the previous fit's center. Changes: the fitted center now lives in c_ while c stays configuration; a user-supplied center is validated (width, finiteness, not all zeros) and copied so a later mutation cannot change the fitted estimator; l2_regularizer defaults to 5e-7 and a new learning_rate defaults to 1e-4 (both match the reference implementation; the l2_regularizer default change is API-visible); the best-epoch snapshot is applied before scoring. Measured on the 17-dataset benchmark: mean ROC AUC 0.601 -> 0.748 with score collapse eliminated on all 17. Calibration: 0.748 is level with the 0.746 of the untrained network, so this restores the baseline rather than improving on it. Deep SVDD assumes a clean one-class training set while PyOD fits it unsupervised on contaminated data; training on genuinely normal samples only reaches 0.879. Documented in the class docstring and CHANGES. Tests: 7 new, each verified to fail on the pre-fix code where applicable -- score-diversity (the collapse detector, both standard and AE paths), center validity, sklearn-contract (c untouched, get_params/clone clean), refit re-initialization, all-zero rejection, and custom-center validation/copying. test_fit_changes_parameters is scoped to the original missing-backward defect and documented as not detecting collapse. Also fixes the README icon to an absolute URL so it renders on PyPI (#705). Reviewed via /implement-review (Codex, 3 rounds: Block -> Block -> shippable).
Problem
In
DeepSVDD.fit()the training loop hadloss.backward()commented out (issue #606):With no backward pass, gradients are never computed, so
optimizer.step()is a no-op — the network never learns and the anomaly scores come from the randomly initialised weights.Simply uncommenting the line raises the error reported in #606:
because the L2 penalty term
w_dwas built once before the epoch loop and then added to every batch's loss, so the secondbackward()re-traverses its already-freed graph.Fix
w_dinside the batch loop, so it belongs to the current batch's graph (resolves the double-backward error).loss.backward().epoch_lossaccumulates over the whole epoch, so comparing it tobest_lossafter every batch saved an early-in-epoch state instead of the best epoch.Verification
On synthetic data (
generate_data, 10 features, 10% contamination), scoring on a held-out set:The training loss now decreases smoothly instead of staying flat,
pyod/test/test_deepsvdd.pypasses (15/15), and ROC AUC is unaffected (~0.998).Note
best_model_dictis still not loaded back intomodel_bydecision_function(scoring uses the final-epoch weights). I left that behaviour unchanged so this PR stays focused on the training bug, but I'm happy to wire it in as a follow-up if you'd like.