add type hints to mne/_fiff/ module - #14278
Conversation
|
I think any change to typing should carry a |
Oops. Totally agree and meant to do that; had been locally running ty manually on the changed files. |
| class SetChannelsMixin(MontageMixin): | ||
| """Mixin class for Raw, Evoked, Epochs.""" | ||
|
|
||
| from ..bem import ConductorModel |
There was a problem hiding this comment.
in local doc builds I am hitting sphinx-doc/sphinx#8664
I thought it would maybe go away if I did this:
| 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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
(it'll show up as a class attribute if you do that I think)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
drammock
changed the title
larsoner
left a comment
larsoner
left a comment
There was a problem hiding this comment.
Just a few minor comments / ideas (that may or may not make sense), otherwise LGTM!
|
|
||
| # 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) |
There was a problem hiding this comment.
Why not just
| info = create_info(ch_names, cast(float, sfreq), ch_types=ch_types) | |
| info = create_info(ch_names, float(sfreq), ch_types=ch_types) |
?
There was a problem hiding this comment.
... 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.
| FIFF.FIFF_UNITM_F = -15 | ||
| FIFF.FIFF_UNITM_A = -18 | ||
| _ch_unit_mul_named = { | ||
| _ch_unit_mul_named: dict[NamedInt, NamedInt] = { |
There was a problem hiding this comment.
I would have expected this to be
| _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
| unit_changes[this_change].append(ch_name) | ||
| # reset unit multiplication factor since the unit has now changed | ||
| info["chs"][c_ind]["unit_mul"] = _ch_unit_mul_named[0] | ||
| info["chs"][c_ind]["unit_mul"] = _ch_unit_mul_named[cast(NamedInt, 0)] |
There was a problem hiding this comment.
... like this one, doesn't seem like you should need to cast
2 participants
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/