mne-python icon indicating copy to clipboard operation
mne-python copied to clipboard

Reading and writing TFRs is confusing

Open cbrnr opened this issue 6 months ago • 3 comments

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!

cbrnr avatar Dec 12 '23 11:12 cbrnr