Skip to content

Add pull request templates with an exhaustive new-model checklist - #1169

Merged
bruAristimunha merged 5 commits into
braindecode:masterfrom
qinxwew:add-pr-templates
Sep 29, 2026
Merged

bruAristimunha merged 5 commits into
braindecode:masterfrom
qinxwew:add-pr-templates

Conversation

@qinxwew

@qinxwew qinxwew commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Closes #913.

Motivation

#913 asks for "an exhaustive todo list for adding models ... directly included in a PR template". The Adding a model to Braindecode guide in CONTRIBUTING.md covers the implementation conventions, but a contributor currently discovers the remaining requirements only through CI failures or by reverse-engineering recent model PRs:

  • several conventions are enforced by test/unit_tests/models/test_integration.py without being listed anywhere upfront (self.final_layer naming, activation exposed as a class-default parameter, drop-probability parameters, summary.csv completeness);
  • the registration and documentation steps (registry in models/util.py, export in models/__init__.py, summary.csv, docs/api.rst, architecture figure) are only visible in past model PRs such as Adding ZUNA to Braindecode #1020;
  • benchmark evidence supporting the reported numbers has become an expectation in model reviews (Make more official way to validate models #1156), but nothing asks for it up front.

I hit exactly these gaps while preparing the MSCFormer addition for #721 — the conventions first surfaced as local test failures — which motivated this PR.

Changes

  • .github/PULL_REQUEST_TEMPLATE/add_new_model.md: template for model additions with the exhaustive checklist (model information, implementation conventions annotated with the enforcing test names, registration and documentation steps, validation and benchmark expectations).
  • .github/PULL_REQUEST_TEMPLATE/default.md: minimal general template (summary / changes / testing / notes). Two templates in PULL_REQUEST_TEMPLATE/ make GitHub show a template chooser; with a single file, GitHub would silently apply the model checklist to every PR, which is not intended.
  • CONTRIBUTING.md: one-line pointer from the "Add a model" section to the template.
  • docs/whats_new.rst: entry for the changelog check.

Design notes

  • The checklist deliberately mirrors the guide rather than replacing it: it points contributors to CONTRIBUTING.md for the how and serves as the what to tick.
  • The benchmark section is phrased as "reproduce the paper's results on at least one public dataset, or explain the gap" — a form-level ask that leaves the broader policy discussion (Make more official way to validate models #1156) to maintainers.
  • No new dependencies; markdown/rst-only changes.

Testing

  • No code changes. Both templates have valid YAML frontmatter (name / description / title), and the chooser behavior was verified to require two or more templates in the directory.
  • codespell and sphinx-lint pass on the touched files (docstrfmt excludes whats_new.rst in pre-commit by design; verified the exclusion before relying on it).
  • Changelog check satisfied via the whats_new.rst entry.

Closes braindecode#913: turn the requirements for adding a model into a checklist that
contributors see when opening the pull request. The implementation
conventions enforced by test_integration.py, the registration and
documentation steps, and the benchmark expectations were previously only
discoverable through CI failures or by reading past model PRs.

A minimal general template is added alongside so GitHub shows a template
chooser; a single template would be applied to every pull request.
@codecov

codecov Bot commented Sep 14, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.19%. Comparing base (15561d1) to head (f6a78d0).
⚠️ Report is 11 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1169      +/-   ##
==========================================
+ Coverage   86.79%   87.19%   +0.39%     
==========================================
  Files         143      146       +3     
  Lines       16386    16896     +510     
==========================================
+ Hits        14223    14733     +510     
  Misses       2163     2163              
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

bruAristimunha and others added 2 commits September 21, 2026 21:12
Resolve the docs/whats_new.rst conflict by keeping both changelog
entries (BrainBERT :gh:`1104` from master, PR templates :gh:`1169`
from this branch).
@qinxwew

qinxwew commented Sep 25, 2026

Copy link
Copy Markdown
Contributor Author

Merged the current master into this branch to clear a conflict introduced by the recently merged #1104 (BrainBERT), which appended a changelog entry in docs/whats_new.rst right where this PR adds its own :gh:1169`` entry.

Resolution kept both entries (BrainBERT's first, matching master, then this PR's), so the resulting diff against master is still only the four files this PR touches:

 .github/PULL_REQUEST_TEMPLATE/add_new_model.md | 57 ++++++++++++++++++++++++++
 .github/PULL_REQUEST_TEMPLATE/default.md       | 23 +++++++++++
 CONTRIBUTING.md                                |  2 +
 docs/whats_new.rst                             |  4 ++

No content changes beyond the changelog ordering.

@qinxwew

qinxwew commented Sep 28, 2026

Copy link
Copy Markdown
Contributor Author

Hi @bruAristimunha — a gentle follow-up on this one, now that it has been two weeks since you kindly merged master into the branch (thank you again for that).

Current status:

  • Everything is green except test (macos-latest, 3.12), which the GitHub runner cancelled at ~87% — no assertion and no traceback in the log, just the shutdown. I cannot re-trigger it from a fork (403 Must have admin rights), so it is sitting there as state: unstable with mergeable: true. test (macos-latest, 3.13) passed on the same commit.
  • The diff is four files and no Python code: the two .github/PULL_REQUEST_TEMPLATE/ templates, a pointer from CONTRIBUTING.md, and the whats_new line. The add_new_model.md checklist is transcribed from the current registry/implementation conventions (including the final_layer and activation-class-default rules I hit while adding MSCFormer), so it should not drift from what the code actually enforces.

This closes #913, which @PierreGtch opened in January for turning the "add a model" TODO list into a PR template. Happy to reword or drop any checklist item if one reads wrong.

@bruAristimunha
bruAristimunha merged commit 1edbf60 into braindecode:master Sep 29, 2026
11 checks passed
@bruAristimunha

Copy link
Copy Markdown
Collaborator

many thanks to attack this issue @qinxwew 🙏🏽

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.

TODO list for adding models

2 participants