mne-tools / mne-tools/mne-python

Reading and writing TFRs is confusing

Open
#12,290 3 comments 1 reaction 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Python
Stars
3.5k
Forks
1.6k
Avg merge
1d 6h
Merged PRs (30d)
100

Description

I thought about asking in our forum, but I decided to open an issue, because I think this is at least partly a documentation issue:

I am confused by our TFRs API. In particular, it is not clear to me how reading and writing is supposed to work. There are two functions, mne.time_frequency.read_tfrs() and mne.time_frequency.write_tfrs().

  1. Let's start with write_tfrs(). According to its docstring, I can pass AverageTFR | list of AverageTFR | EpochsTFR – a list of EpochsTFR is not supported, but I can still pass it without any problems. So I guess the line in the docstring should be adapted to AverageTFR | list of AverageTFR | EpochsTFR | list of EpochsTFR.

    BTW, and this should probably be a separate discussion, but I really think we should use type hint syntax in our docstrings. For example, the previous docstring should really be changed to AverageTFR | list[AverageTFR] | EpochsTFR | list[EpochsTFR].

  2. I have no idea what this note in the description of the tfr parameter means: "Note. If .comment is not None, a name will be generated on the fly, based on the order in which the TFR objects are passed."

  3. Similarly, the docstring of read_tfrs() says that the function returns AverageTFR | list of AverageTFR | EpochsTFR, so this is probably also missing a list of EpochsTFR.

  4. The docstring also says that the return value of read_tfrs() depends on condition: "Depending on condition either the TFR object or a list of multiple TFR objects." This is only partly true, because I also get a list of AverageTFR if I read a file containing only a single AverageTFR object (same for a single EpochsTFR object). So this comment needs to be adapted, or maybe even better, the function should always return a list (of EpochsTFR or AverageTFR), even when there is only a single TFR.

  5. I don't understand the condition parameter. How are conditions stored in AverageTFR objects?

  6. It seems like condition does not work for EpochsTFR, this should be documented. And because I don't understand it in the first place, why does it not work for EpochsTFR?

  7. The types of condition should be changed to int | str | list of int | list of str | None.

Sorry for this long list!

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with the read_tfrs() and write_tfrs() API documentation and their docstrings. Check the documented and observed behavior for TFR lists, generated names, return values, conditions, and EpochsTFR; done means the supported types and behavior are explained consistently.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
documentation
Issue type
Documentation
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.