add type hints to mne/_fiff/ module by drammock · Pull Request #14278 · mne-tools/mne-python

drammock · 2026-09-04T22:18:14Z

opening as draft PR so folks can take a look and tell me what I did wrong, before I move on to doing the remaining files in mne/_fiff/

larsoner · 2026-09-04T22:32:31Z

I think any change to typing should carry a ty change that allowlists (or un-blocklists) the files if possible. I think at the end of the day we can lean on ty plus our existing unit tests (including the test_docstring_parameters to make sure things make sense and are correctly applied

drammock · 2026-09-05T00:31:29Z

I think any change to typing should carry a ty change that allowlists (or un-blocklists) the files if possible.

Oops. Totally agree and meant to do that; had been locally running ty manually on the changed files.

drammock · 2026-09-11T20:19:25Z

 class SetChannelsMixin(MontageMixin):
    """Mixin class for Raw, Evoked, Epochs."""

+    from ..bem import ConductorModel


in local doc builds I am hitting sphinx-doc/sphinx#8664

I thought it would maybe go away if I did this:

Suggested change

from ..bem import ConductorModel

from ..bem import ConductorModel #: :no-index:

But that didn't work. @larsoner have you ever hit:

WARNING: duplicate object description of mne.bem.ConductorModel, other instance in generated/mne.Epochs, use :no-index: for one of them

it's showing up for Epochs, EpochsArray, Evoked, EvokedArray

same in CI: https://app.circleci.com/pipelines/github/mne-tools/mne-python/34359/workflows/b793efbb-e3d9-45a8-aca8-78eebdfb1df2/jobs/87974?invite=true&expanded=true#step-163-6984_148

turns out I can prevent it by turning the type hint into a string --- which is a shame because you lose readability / comments about what each type hint represents (radius, xyz+radius, etc). I'll push that change though in the interest of getting things green.

Did you mean to put it inside the class as an attribute by importing it within the class :? I think that's what Sphinx is confused about probably

(it'll show up as a class attribute if you do that I think)

I agree with your diagnosis, but I'd expect the #: :no-index: trick to work in that case. I nested it inside the class because I can't put it top-level due to our module nesting/hierarchy rules.

I was able to get rid of the errors by stringifying the type hint (along with tweaks to the corresponding numpydoc parameter type description). I dislike doing that especially for such a long/heterogeneous hint, but if it's necessary I'll get over it.

I think having it there is worse than a sphinx warning, because won't it then be exposed as SetChannelsMixin.ConductorModel? That is a very strange thing to have happen. So string is better (or some import hierarchy restructuring is better) I think than nesting the import inside the class def

yeah the nested ConductorModel import is gone now (and the sphinx warning with it).

Windows pip-pre is failing (404 on getting vtk wheels, probably transient) but I expect the rest to pass.

larsoner

Just a few minor comments / ideas (that may or may not make sense), otherwise LGTM!

larsoner · 2026-09-12T14:16:11Z


    # Create mne structure
-    info = create_info(ch_names, sfreq, ch_types=ch_types)
+    info = create_info(ch_names, cast(float, sfreq), ch_types=ch_types)


Why not just

Suggested change

info = create_info(ch_names, cast(float, sfreq), ch_types=ch_types)

info = create_info(ch_names, float(sfreq), ch_types=ch_types)

?

... and actually I would expect create_info to take any numeric thing (including int) and cast to float internally... so it seems like the cast shouldn't be necessary at all.

TL;DR: the problem isn't int-like things, it's None.

A quirk of the hitachi reader is that sfreq is initialized to None and only changed in an if clause within a for-loop over file header lines. Thus the type checker can't be certain it was ever changed from None into a float.

thus when I apply your suggested change, I get

^^^^^ Expected `str | Buffer | SupportsFloat | SupportsIndex`, found `None | float`

I don't think we should change create_info to accommodate this case (allowing sfreq=None as input is misleading, and at best just means we raise an error later rather than sooner)

Yeah let's just not allow None here. Can initialize Hitachi to a dummy val like 1000. and then change it afterward. create_info should only accept numeric values (int-like, float-like)

(incidentally, I think this is one of the advantages of this typing work -- my sense is that it generally forcing us to be slightly cleaner with our code at the end of the day, even if we have to jump through a few more hoops)

hitachi sfreq initialized to float in a54b65d

I think this one is ready now @larsoner

larsoner · 2026-09-12T14:18:27Z

 FIFF.FIFF_UNITM_F = -15
 FIFF.FIFF_UNITM_A = -18
-_ch_unit_mul_named = {
+_ch_unit_mul_named: dict[NamedInt, NamedInt] = {


I would have expected this to be

Suggested change

_ch_unit_mul_named: dict[NamedInt, NamedInt] = {

_ch_unit_mul_named: dict[int, NamedInt] = {

NamedInt subclasses int so you should still be able to use it as a key (I think?) and it could maybe avoid some cast calls

you're right about this one; I had initially done it your way but changed it to NamedInt,NamedInt when trying to rule out causes of some typing error. Fixed now.

larsoner

Awesome, thanks @drammock !

…d-sphere * commit '0311e444b0450ce9d2b2b304cebe7d5343533357': Fix static doc viewing in IDEs (mne-tools#14195) Fix COLA for odd sizes (mne-tools#14312) add type hints to mne/_fiff/ module (mne-tools#14278) ENH: wire the JupyterLite build into the docs (JupyterLite split 5/5) (mne-tools#14157) MAINT: Update pre-commit hook versions (mne-tools#14306) Small documentation fix for "Frequency and time-frequency sensor analysis" tutorial (mne-tools#14304) FIX: don't mutate cached ICA sources in plot_sources properties (mne-tools#14303) Fix bug with lite backend closing (mne-tools#14302) Fix for NumPy eig change [circle deploy] (mne-tools#14300) No PyQt5 [circle deploy] [skip azp] [skip actions] ENH: add the JupyterLite notebook setup cell (JupyterLite split 4/5) (mne-tools#14150) FIX: Ver [circle deploy] [skip azp] [skip actions] Hotfix fix EDF round-trip (mne-tools#14296) Brain GUI modernization (Phase 7) (mne-tools#14270) Fix bug with SciPy 1.18.0 EEGLAB reading (mne-tools#14293) remove plotting abs as the defauls for plotting volumetric (mne-tools#13989) Coregistration GUI modernisation (phase 4) (mne-tools#14285) Dipole GUI interactivity and visual enhancements (phase 2) (mne-tools#14291)

* upstream/main: (91 commits) Fix static doc viewing in IDEs (mne-tools#14195) Fix COLA for odd sizes (mne-tools#14312) add type hints to mne/_fiff/ module (mne-tools#14278) ENH: wire the JupyterLite build into the docs (JupyterLite split 5/5) (mne-tools#14157) MAINT: Update pre-commit hook versions (mne-tools#14306) Small documentation fix for "Frequency and time-frequency sensor analysis" tutorial (mne-tools#14304) FIX: don't mutate cached ICA sources in plot_sources properties (mne-tools#14303) Fix bug with lite backend closing (mne-tools#14302) Fix for NumPy eig change [circle deploy] (mne-tools#14300) No PyQt5 [circle deploy] [skip azp] [skip actions] ENH: add the JupyterLite notebook setup cell (JupyterLite split 4/5) (mne-tools#14150) FIX: Ver [circle deploy] [skip azp] [skip actions] Hotfix fix EDF round-trip (mne-tools#14296) Brain GUI modernization (Phase 7) (mne-tools#14270) Fix bug with SciPy 1.18.0 EEGLAB reading (mne-tools#14293) remove plotting abs as the defauls for plotting volumetric (mne-tools#13989) Coregistration GUI modernisation (phase 4) (mne-tools#14285) Dipole GUI interactivity and visual enhancements (phase 2) (mne-tools#14291) Width [circle deploy] [skip azp] [skip actions] ...

drammock added 2 commits September 4, 2026 17:15

type hint _fiff/meas_info.py

95e1c39

simpler

e3a1fbb

drammock added 3 commits September 8, 2026 10:47

nest bem

dcd8975

one more

a0049f6

add to ty config

c02fc2e

drammock requested a review from larsoner as a code owner September 8, 2026 15:52

drammock added 3 commits September 8, 2026 16:26

add Sequence to numpydoc xref aliases

d904193

docstring <-> typehint fixups

f9f1fb2

fix array shape/dtype annotations

e7c5280

drammock requested a review from agramfort as a code owner September 8, 2026 21:40

don't xref words

91c90ea

drammock added the no-changelog-entry-needed label Sep 11, 2026

drammock commented Sep 11, 2026

View reviewed changes

drammock added 4 commits September 11, 2026 15:44

put them back

46e8917

stringify type hint

ee17f42

propogate to Epochs/Evoked

a6dd5c9

a bit cleaner

125b74e

drammock requested a review from mscheltienne as a code owner September 11, 2026 21:31

drammock changed the title ~~WIP: add type hints to mne/_fiff/ module~~ add type hints to mne/_fiff/ module Sep 11, 2026

larsoner reviewed Sep 12, 2026

View reviewed changes

drammock added 2 commits September 14, 2026 10:03

int as key

6093d13

default float sfreq for hitachi

a54b65d

larsoner approved these changes Sep 15, 2026

View reviewed changes

larsoner enabled auto-merge (squash) September 15, 2026 14:34

larsoner merged commit d07ed22 into mne-tools:main Sep 15, 2026
26 checks passed

	from ..bem import ConductorModel
	from ..bem import ConductorModel #: :no-index:

	info = create_info(ch_names, cast(float, sfreq), ch_types=ch_types)
	info = create_info(ch_names, float(sfreq), ch_types=ch_types)

	_ch_unit_mul_named: dict[NamedInt, NamedInt] = {
	_ch_unit_mul_named: dict[int, NamedInt] = {

Uh oh!

Conversation

drammock commented Sep 4, 2026

Uh oh!

larsoner commented Sep 4, 2026

Uh oh!

drammock commented Sep 5, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

larsoner Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

larsoner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

larsoner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

larsoner Sep 11, 2026 •

edited

Loading