Eden-Kramer-Lab / spectral_connectivity

Frequency domain estimation and functional and directed connectivity analysis tools for electrophysiological data
GNU General Public License v3.0
121 stars 43 forks source link

Xarray wrapper #16

Closed mmyros closed 3 years ago

mmyros commented 3 years ago

spectral_connectivity.multitaper_connectivity() uses Multitaper and Connectivity and returns xarray output. This allows for one-liner faceted plots. See example at the end of Tutorial_On_Simulated_Examples.ipynb

coveralls commented 3 years ago

Coverage Status

Coverage increased (+1.5%) to 76.117% when pulling fdae7d8044f1fc8be759417781b213b4feba3b5b on mmyros:master into e95cb0cd696ead2da0b52ac1f569df068c750e6f on Eden-Kramer-Lab:master.

edeno commented 3 years ago

Hi @mmyros, this is great. I really appreciate this addition. It is something I've meant to get around to for a long time and I think it really improves the usability of the package.

Couple of thoughts:

  1. There should probably be two kwargs arguments. One for the multitaper and one for connectivity. The reason for this is a few of the methods such as Canonical Coherence take additional arguments.
  2. It would be great if we could include some of the attributes from Multitaper as attributes in the xarray object. The important ones are: frequency resolution, time window duration, time window step, and sampling frequency. I believe it is important for anyone analyzing spectral data to be aware of these parameters. This is more of a wish than a request, because I can always add these myself later.

What do you think?

mmyros commented 3 years ago

Hi Eric, I'm glad this is helpful! Great suggestions, I will try to implement one or both of these.

For #1, maybe it's best to create a separate function for multitaper + connectivity that takes two sets of kwargs. The xarray part only really needs the output array and MT object, so it can be a separate function for those who prefer to calculate those separately. I'm just concerned that including that many arguments will have a high cognitive complexity.

Do you think #2 could be done with super()? Otherwise I can try adding them as coordinates.

edeno commented 3 years ago

I do think a separate function is best. I initially separated the multitaper transform and connectivity because I was thinking of implementing a wavelet transform as well.

Perhaps for 1, it could be a decorator for all the connectivity functions? That way everything gets spit out as a xarray object. Again, more aspirational though, just having a function to start with is great.

And for 2, I would just propagate them as attributes on the xarray DataArray (See http://xarray.pydata.org/en/stable/data-structures.html)

mmyros commented 3 years ago

Decorator sounds intriguing. I'm looking forward to your comments re: 2900e7f as that implements attributes and separates out multitaper_connectivity and connectivity_to_xarray. multitaper_connectivity now calls connectivity_to_xarray()

mmyros commented 3 years ago

This iteration skips group_delay and canonical_coherence, since canonical_coherence has one more dimension than the rest. This can of course be fixed, but maybe at a later time. Could you make sure that the rest of the outputs checks out? Thanks for the comments!

edeno commented 3 years ago

Thanks @mmyros !