mne-tools / mne-python

MNE: Magnetoencephalography (MEG) and Electroencephalography (EEG) in Python
https://mne.tools
BSD 3-Clause "New" or "Revised" License
2.7k stars 1.31k forks source link

Glossary: add remaining sensor types and more referencing #12490

Closed nabilalibou closed 6 months ago

nabilalibou commented 7 months ago

Proposed documentation enhancement

A few suggestions to add clarity around the many different types of channel which can be called sensor types, including the sub-denomination of data channels:

=> Change the pre-existing data channels section from the Glossary (data channels) to be included in a global section sensor types. The sensor types section will contain an additional subsection other channel listing other channel types (the non-brain ones).

=> Add a link referencing the glossary term sensor types inside the note in the documentation of the set_channel_types method (for all data classes).

=> Add all other non listed sensor types and add a link to sensor types in this tutorial paragraph: https://mne.tools/stable/auto_tutorials/raw/10_raw_overview.html#changing-channel-name-and-type

The method takes a dictionary mapping channel names to types; allowed types are ecg, eeg, emg, eog, exci, ias, misc, resp, seeg, dbs, stim, syst, ecog, hbo, hbr.

larsoner commented 7 months ago

I'd prefer to keep data channels as a standalone glossary entry. One way to do this would be to keep data channels but add non-data channels that lists all other sensor types, and then sensor types which just links to these two entries saying every sensor is data or non-data or similar

nabilalibou commented 7 months ago

Ok so I should open a new PR to:

larsoner commented 7 months ago

Sounds good

nabilalibou commented 7 months ago

Hey @larsoner, one question before i open the PR:

I am thinking that it would be optimal to use the class MNESubstitution you created in https://github.com/mne-tools/mne-python/tree/main/doc/sphinxext/mne_substitutions.py by adding a piece of code to run with the argument 'non-data channels'. I will therefore also be relying on the various constants from https://github.com/mne-tools/mne-python/tree/main/mne/_fiff/pick.py#L1095 and the metadata from https://github.com/mne-tools/mne-python/blob/main/mne/defaults.py.

For metadata, I also wanted to use this dict but it does not contain all the 31 sensor types. Five non-data channel types are missing from the dict as well as the Units dict even though they exist in scaling_plot_raw and Color dict.

These types are:

Should these types be added to the dictionaries where they're missing, or is this done on purpose and I should hardcode them in MNESubstitution code ?

By the way, channel_types could be updated with all the missing types too ?

larsoner commented 7 months ago

Five non-data channel types are missing from the dict as well as the Units dict even though they exist in scaling_plot_raw and Color dict.

Yes there are inconsistencies in some of these constants and IIRC in some GitHub issue (or maybe in code comments?) there are some # XXX or # TODO comments talking about unifying them or make them more consistent.

It would be great to make them consistent but might (or might not!) be quite difficult, because they're used not just for I/O but also for plotting. If you're motivated to try it, I'd suggest by opening a PR with what you think would be the smallest, easiest one step toward unification / consistency and see what happens in tests. Then we can see how badly things break, if at all. Some tests might need to be updated, etc. I think we'd want to proceed as incrementally as possible to be safe, but it's possible multiple constants will need to change simultaneously... but we'll see!

nabilalibou commented 7 months ago

Did I step down a rabbit hole haha

Ok, thanks for the tips, I willl start by looking at all the issues and comments with information about this. Then I willl do a PR draft where I willl go step by step and check off the boxes one by one, gradually and steadily.

larsoner commented 7 months ago

Did I step down a rabbit hole haha

Perhaps :smile: