mne-tools / mne-connectivity

Connectivity algorithms that leverage the MNE-Python API.
https://mne.tools/mne-connectivity/dev/index.html
BSD 3-Clause "New" or "Revised" License
68 stars 34 forks source link

High level design plan: What belongs in which package? #57

Open adam2392 opened 2 years ago

adam2392 commented 2 years ago

Summary

We had a chat with the Frites team and other independent developers on 11/29/21 to develop a robust Python suite of connectivity based analysis. To enable this, we agreed that on a high level that:

This then should keep in line w/ the resources dedicated to both projects: mne-connectivity can move faster then mne-python and Frites can move faster then both.

PR Plan

Here, we should enumerate the PRs necessary to bring this to completion once we both review the code in frites and mne-connectivity.

Lmk If I missed anything.

cc: @brovelli @adam2392 @EtienneCmb @ViniciusLima94 @brovelli @jshanna100

adam2392 commented 2 years ago

TODO list for MNE-Connectivity

  1. [ ] a PR to the docs page pointing to Frites
  2. [ ] a PR to enable exporting of all Connectivity data structures to data frame (http://xarray.pydata.org/en/stable/generated/xarray.DataArray.to_dataframe.html). Probably just need to add this function wrapper to _Connectivity base class, or just add an example demonstrating it.
  3. [ ]

more to come...

EtienneCmb commented 2 years ago

Hi @adam2392,

Thanks for starting the discussion.

From our side we could add a function conn_frites_to_mne for converting to your base objects. Something like that : conn_frites_to_mne.

I could also add a input parameter to_mne=True/False to each frites.conn function for returning the outputs into MNE format. Something like :

# conversion to mne-connectivity base object
if to_mne:
    return conn_frites_to_mne(conn)

What do you think?

adam2392 commented 2 years ago

Container Discussion

^ tagging this as a container discussion.

What do you think?

Thanks! Overall this is definitely in the right direction I think. See below for specific thoughts.

From our side we could add a function conn_frites_to_mne for converting to your base objects. I could also add a input parameter to_mne=True/False to each frites.conn function for returning the outputs into MNE format. Something like :

I think my only concern here is the maintainability of such a standard. Say the connectivity containers are evolving and allow additional functionality, a PR would be needed to each frites.conn function to update them accordingly. It could also be missed. A way to counteract this is of course sufficient unit-tests that are versioned w/ the current main branch of mne-connectivity.

proposal for container consolidation

I was looking over the Subject and Group containers in frites. I am wondering for the subject-level containers, is there any difference between that and any of the containers in mne? I'm wondering if there's a way we can just consolidate the containers in one package, so we don't need the if/else cases? Of course, then we can add GroupLevel containers on mne side.

The only missing data points I can see are the "subject name", "agg_ch" and "multivariate" parameters in https://brainets.github.io/frites/api/generated/frites.dataset.SubjectEphy.html. Not 100% sure of their functionality though. If they're easy enough to port, this can then simplify the frites codebase?

Then, for example https://brainets.github.io/frites/api/generated/frites.conn.conn_dfc.html can return either the xarray, or 1 connectivity container.