Repository navigation
Refactor shared EMG model support components - #1145
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors and centralizes shared sEMG baseline support components so existing models (Dance, EMG2Qwerty) and upcoming pose baselines can reuse common head-reset logic, TDS encoder blocks, and SMU activation.
Changes:
- Introduces reusable TDS encoder building blocks (
TDSConv2dBlock,TDSFullyConnectedBlock,TDSConvEncoder) and adds unit tests for layout equivalence + TorchScript. - Adds
SmoothMaximumUnitactivation and exports new shared components viabraindecode.modules. - Centralizes
n_outputsbookkeeping viaEEGModuleMixin._set_n_outputsand updates Dance/EMG2Qwerty to use it.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/unit_tests/models/test_modules.py | Adds focused tests for TDS blocks (layout equivalence + TorchScript). |
| braindecode/modules/blocks.py | Adds shared TDS convolution/FC blocks and encoder implementation. |
| braindecode/modules/activation.py | Adds SmoothMaximumUnit activation. |
| braindecode/modules/init.py | Exposes new activation and TDS blocks as public module exports. |
| braindecode/models/emg2qwerty.py | Migrates to shared TDS encoder and centralizes head reset bookkeeping. |
| braindecode/models/dance.py | Migrates head reset bookkeeping to _set_n_outputs. |
| braindecode/models/base.py | Adds _set_n_outputs helper to keep configs and n_outputs synchronized. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| self._set_n_outputs(n_outputs) | ||
| old_head = self.final_layer | ||
| self.final_layer = nn.Linear(old_head.in_features, n_outputs).to( | ||
| device=old_head.weight.device, dtype=old_head.weight.dtype | ||
| ) |
| @@ -359,10 +358,3 @@ def reset_head(self, n_outputs: int) -> None: | |||
| self.decoder.class_head = nn.Linear(ch.in_features, n_outputs).to( | |||
| device=ch.weight.device, dtype=ch.weight.dtype | |||
| ) | |||
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 621c874a73
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) | ||
|
|
||
|
|
||
| class TDSConv2dBlock(nn.Module): |
There was a problem hiding this comment.
These classes are extracted from emg2qwerty.py, whose implementation is explicitly inherited under CC BY-NC-SA 4.0, but placing them in modules/blocks.py makes them subject to the repository's default BSD-3 declaration: NOTICE.txt:7-33 lists only models/emg2qwerty.py as the relevant license exception. Consequently, users importing the newly public TDS blocks are told they may use them commercially and without ShareAlike obligations. Keep the extracted implementation in a separately attributed CC BY-NC-SA file and add it to the notice, or otherwise preserve the inherited licensing metadata.
Useful? React with 👍 / 👎.
| - Add reusable temporal-distributed separable convolution blocks and the | ||
| smooth maximum unit activation to :mod:`braindecode.modules`, and centralize | ||
| output-head replacement for models using | ||
| :class:`braindecode.models.base.EEGModuleMixin` (:gh:`1145` by `Bruno | ||
| Aristimunha`_) |
| - Add reusable temporal-distributed separable convolution blocks and the | ||
| smooth maximum unit activation to :mod:`braindecode.modules`, and centralize | ||
| output-head replacement for models using | ||
| :class:`braindecode.models.base.EEGModuleMixin` (:gh:`1145` by `Bruno | ||
| Aristimunha`_) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (1)
docs/whats_new.rst:31
- The release note calls this a "temporal-distributed separable convolution" encoder, but the implementation and naming (TDS = time-depth-separable) indicate it should be described as a time-depth-separable convolution encoder to avoid confusing terminology.
- Add a reusable temporal-distributed separable convolution encoder to
Summary
Extract the reusable support pieces needed by the sEMG pose baselines into a focused prerequisite PR:
EEGModuleMixinThis is the support layer for #1132. Keeping it separate lets #1132 contain the three pose models and their model-specific integration only.
Verification
python -m pytest test/unit_tests/models/test_modules.py -q(206 passed)python -m pytest test/unit_tests/models/test_models.py -k emg2qwerty -q(8 passed)python -m pytest test/unit_tests/models/test_models.py -k dance -q(2 passed)