mne-tools / mne-tools/mne-python
Reading and writing TFRs is confusing
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().
-
Let's start with
write_tfrs(). According to its docstring, I can passAverageTFR | list of AverageTFR | EpochsTFR– a list ofEpochsTFRis not supported, but I can still pass it without any problems. So I guess the line in the docstring should be adapted toAverageTFR | 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]. -
I have no idea what this note in the description of the
tfrparameter 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." -
Similarly, the docstring of
read_tfrs()says that the function returnsAverageTFR | list of AverageTFR | EpochsTFR, so this is probably also missing alist of EpochsTFR. -
The docstring also says that the return value of
read_tfrs()depends oncondition: "Depending onconditioneither the TFR object or a list of multiple TFR objects." This is only partly true, because I also get a list ofAverageTFRif I read a file containing only a singleAverageTFRobject (same for a singleEpochsTFRobject). So this comment needs to be adapted, or maybe even better, the function should always return a list (ofEpochsTFRorAverageTFR), even when there is only a single TFR. -
I don't understand the
conditionparameter. How are conditions stored inAverageTFRobjects? -
It seems like
conditiondoes not work forEpochsTFR, this should be documented. And because I don't understand it in the first place, why does it not work forEpochsTFR? -
The types of
conditionshould be changed toint | str | list of int | list of str | None.
Sorry for this long list!
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- 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