Skip to content

shifter init fix - #247

Merged
pollytur merged 3 commits into
sinzlab:mainfrom
NathanSoeding:shifter-init
Jul 6, 2026
Merged

shifter init fix#247
pollytur merged 3 commits into
sinzlab:mainfrom
NathanSoeding:shifter-init

Conversation

@NathanSoeding

Copy link
Copy Markdown
Contributor

shifter initialization function was never called and it checked if parameters are instances of nn.Linear which is always wrong because they are tensors.

Comment thread neuralpredictors/layers/shifters/mlp.py Outdated
Comment thread neuralpredictors/layers/shifters/mlp.py Outdated

@pollytur pollytur left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NathanSoeding thanks a lot for the PR!

see the comments to address

Ideally please add a few unit tests to check that initialization now works correctly for both MLP(Shifter) and class MLPShifter(ModuleDict) to make sure we are not missing any other bugs :)

@pollytur

pollytur commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator

@NathanSoeding any updates on it? It would be great if we could finalise it soon :)

@NathanSoeding

Copy link
Copy Markdown
Contributor Author

I committed the changes

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes MLP shifter weight initialization by ensuring the initialization routine is actually invoked and by correctly initializing nn.Linear layer weights (instead of incorrectly checking Parameter objects as nn.Linear). It also adds a regression test to ensure Xavier initialization is used.

Changes:

  • Call MLP.initialize() from MLP.__init__ and correctly apply Xavier init to nn.Linear layers (plus zero-initialize biases).
  • Remove the previous (incorrect) parameter-based initialization logic.
  • Add a unit test that patches xavier_normal_ and asserts it is called once per Linear layer.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
neuralpredictors/layers/shifters/mlp.py Fixes and triggers MLP shifter initialization; changes init behavior and removes container-level init method.
test/test_shifter_init.py Adds a regression test verifying Xavier initialization is applied for each shift layer.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread neuralpredictors/layers/shifters/mlp.py
Comment thread neuralpredictors/layers/shifters/mlp.py
Comment thread test/test_shifter_init.py Outdated
Comment on lines +1 to +3
import torch
from torch import nn
from unittest.mock import patch

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NathanSoeding please fix this :)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and also maybe add a sweep that you sent several values of the MLP layers and not just hardcoded 3 - in experanto there should be examples how to parametric tests with pytest like here https://github.com/sensorium-competition/experanto/blob/main/tests/test_sequence_interpolator.py#L14-L16

If pytest is not a part of neural predictors - ignore this comment and just remove the unused inputs

Have you run the tests locally?

@NathanSoeding

Copy link
Copy Markdown
Contributor Author

@pollytur I removed unused library imports and added added the module dict initialization function which calls the individual shifter initialize() functions

@pollytur
pollytur merged commit efdda67 into sinzlab:main Jul 6, 2026
1 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants