fix(core): second-pass review findings - #439
Open
ChrisW09 wants to merge 2 commits into
Open
Conversation
- BaseModel.encode(): drop the stray self.encoder(x) call in the no-grad branch. Mamba architectures have no 'encoder' attribute, so the public clf.encode(X) path raised AttributeError for them; for every other model it ran the encoder twice and discarded the first result. The grad=True branch never had the extra call. - _validate_fit_inputs gated the strictly-positive check on 'inversegaussian', which is not in DISTRIBUTION_REGISTRY (the real key is 'inversegamma'), so the check never fired; lognormal needs it too. - sparsemax shifted its input in place. ODST passes the feature_selection_logits Parameter directly, so the stored parameter values were corrupted by -max on first use. Output is shift-invariant, so predictions were unaffected, but weight decay and checkpoints saw the shifted values. - TaskModel now initialises train_features/train_targets to None, so the 'is not None' guards in the step methods work for candidate models restored from a checkpoint without an in-process fit. Fixes #426 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
sparsemax is typed as returning Tensor | None via the autograd Function. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
1 participant
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.
Fixes #426 (items 1, 2, 4 and 5; the CRPS-fallback sigma, the MambaOriginal cache method and the TabTransformer zero-numeric-feature guard are left for separate PRs since each needs a design call).
Problems fixed
BaseModel.encode()stray call. Thegrad=Falsebranch (the default, used by the publicclf.encode(X)mixin) hardcodedembeddings = self.encoder(x)before overwriting it withlayer(x). Mambular / MambAttention / MambaTab exposeself.mambaand noencoder, soencode()raisedAttributeError; every other architecture ran its encoder twice and threw the first result away. The symmetricgrad=Truebranch never had the extra line._validate_fit_inputs. The strictly-positive check gated on"inversegaussian", which does not exist inDISTRIBUTION_REGISTRY(the real key is"inversegamma"), so it never fired — andlognormalneeds the same guard.sparsemaxmutated its input in place.input_ -= max_valinsideFunction.forward; ODST (nn/blocks/node.py) passes itsfeature_selection_logitsParameter directly, so the stored values were shifted by-maxon first use. The output is shift-invariant so predictions were unaffected, but weight decay and saved checkpoints operated on corrupted values.TaskModel.train_featureswas never initialised. The step methods guard with... and self.train_features is not None, which presumes aNonedefault, but the attribute is only created insetup(stage="fit")— so validate/predict on a candidate model restored from a checkpoint raisedAttributeErrorfrom the guard itself.Tests
New
tests/test_second_pass_fixes.py: sparsemax leaves its input untouched (and still returns a valid simplex projection), the positivity check fires for all three strictly-positive families and the checked keys actually exist in the registry, andTaskModelstarts withtrain_features is None.tests/test_exceptions.py,tests/test_nn_blocks.pyandtests/test_models.pypass unchanged (424 passed).🤖 Generated with Claude Code