Skip to content

FIX restore ShallowFBCSPNet split_first_layer=False - #1212

Merged
bruAristimunha merged 4 commits into
braindecode:masterfrom
lindicaphxag-tech:fix/shallow-unsplit-first-layer
Oct 5, 2026
Merged

FIX restore ShallowFBCSPNet split_first_layer=False#1212
bruAristimunha merged 4 commits into
braindecode:masterfrom
lindicaphxag-tech:fix/shallow-unsplit-first-layer

Conversation

@lindicaphxag-tech

@lindicaphxag-tech lindicaphxag-tech commented Oct 3, 2026

Copy link
Copy Markdown
Contributor

Fixes #1211.

The CombinedConv migration left two split-only assumptions unconditional in ShallowFBCSPNet: initialization accesses conv_time_spat.conv_time even when split_first_layer=False, and legacy conv_time.* keys are always remapped through conv_time_spat.

This restores the historical two-mode contract:

  • split first layer: keep the current CombinedConv initialization and legacy key migration;
  • unsplit first layer: initialize the direct conv_time module and keep legacy conv_time.* checkpoint keys on that module;
  • classifier key migration remains unchanged.

The regression is confirmed against Braindecode v0.7.0, where the unsplit path initialized self.conv_time directly.

Regression tests cover:

  • construction and forward pass with split_first_layer=False;
  • presence of conv_time and absence of conv_time_spat;
  • loading a legacy-style state dict while preserving classifier-key migration.

The default split_first_layer=True path is unchanged.

@codecov

codecov Bot commented Oct 3, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.87%. Comparing base (2426ddc) to head (f6ac768).
⚠️ Report is 7 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #1212      +/-   ##
==========================================
+ Coverage   87.84%   87.87%   +0.02%     
==========================================
  Files         151      151              
  Lines       17602    17610       +8     
==========================================
+ Hits        15462    15474      +12     
+ Misses       2140     2136       -4
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@lindicaphxag-tech ChatGPT Codex Connector

lindicaphxag-tech commented Oct 3, 2026

Copy link
Copy Markdown
Contributor Author

Exact-head validation is green on 3695bb36a7c63e316a418e0c8ae4f50c08d8ba99: all touched-file pre-commit hooks pass (including Ruff, mypy, isort, codespell and sphinx-lint), the focused ShallowFBCSP regressions pass (6 passed), and the explicit unsplit + no-batchnorm branch passes separately (1 passed). Evidence: https://github.com/lindicaphxag-tech/lindicaphxag-tech/actions/runs/37136141969. I’ll hold this head for maintainer review.

@lindicaphxag-tech ChatGPT Codex Connector

lindicaphxag-tech commented Oct 4, 2026

Copy link
Copy Markdown
Contributor Author

Superseding my earlier validation note: the current PR head is ec2d069dcf0c4964ad2b2e53147963ad85e0d820, and it has now been revalidated directly. Touched-file pre-commit passes; the focused ShallowFBCSP regressions pass (6 passed), and the explicit split_first_layer=False, batch_norm=False path passes separately (1 passed), including the direct-convolution bias initialization branch. Evidence: https://github.com/lindicaphxag-tech/lindicaphxag-tech/actions/runs/37189282801. I’ll hold the current head for review.

@lindicaphxag-tech
lindicaphxag-tech force-pushed the fix/shallow-unsplit-first-layer branch from 31ff3aa to 02e048f Compare October 4, 2026 15:01
@bruAristimunha
Merge branch 'master' into fix/shallow-unsplit-first-layer
f6ac768 
Resolved docs/whats_new.rst by keeping both changelog entries.
@bruAristimunha bruAristimunha added the maintenance Bug fix / refactor / tests — not a new model label Oct 5, 2026
bruAristimunha
bruAristimunha approved these changes Oct 5, 2026
View reviewed changes

@bruAristimunha bruAristimunha 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.

Thanks for the fix (#1211). Reproduced the construction crash on master; unsplit path and legacy state-dict loading verified; default path bit-identical.

@bruAristimunha
bruAristimunha merged commit 497c0c1 into braindecode:master Oct 5, 2026
11 of 12 checks passed
bruAristimunha added a commit to bruAristimunha/braindecode that referenced this pull request Oct 5, 2026
@bruAristimunha
Merge branch 'master' into fix/from-pretrained-geometry-kwargs
ccbd028 
Conflict resolution:
- docs/whats_new.rst: kept both the braindecode#1232 from_pretrained geometry-kwargs bug-fix entry and master's braindecode#1207/braindecode#1212 Deep4Net/ShallowFBCSPNet bug-fix entries.
- test/unit_tests/models/test_eegdino.py: kept both the braindecode#1232 geometry-kwargs test block and master's braindecode#1194 qkv-hook attention test block.
bruAristimunha added a commit to bruAristimunha/braindecode that referenced this pull request Oct 5, 2026
@bruAristimunha
Merge branch 'master' into fix/cbramod-lazy-head
b3d1ec1 
Conflict resolution:
- braindecode/models/cbramod.py: kept master's braindecode#1226 PatchTokenizer on_non_divisible padding and merged the PR's _knows_geometry() helper (superset of master's _n_times/_n_chans-not-None check) into the PatchTokenizer n_times arg, final_layer construction (both __init__ sites), reset_head, and _n_patch.
- docs/whats_new.rst: kept both the braindecode#1233 CBraMod lazy-head bug-fix entry and master's braindecode#1207/braindecode#1212 Deep4Net/ShallowFBCSPNet bug-fix entries.
- test/unit_tests/models/test_foundation_models.py: kept master's MAPA/PopT test blocks and the PR's test_cbramod_head_is_concrete_when_geometry_is_derived.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bruAristimunha bruAristimunha bruAristimunha approved these changes

Assignees

No one assigned

Labels

maintenance Bug fix / refactor / tests — not a new model

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ShallowFBCSPNet split_first_layer=False is broken by unconditional CombinedConv init/remapping

2 participants

@lindicaphxag-tech @bruAristimunha