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
66 stars 34 forks source link

[ENH] Multiple improvements to spectral_connectivity_time: ciPLV, and efficient computation of multiple metrics #115

Closed ruuskas closed 1 year ago

ruuskas commented 1 year ago

PR Description

This PR addresses improvements mentioned in #104. I'm reposting this with the Git log now cleaned up.

Merge checklist

Maintainer, please confirm the following before merging:

ruuskas commented 1 year ago

It looks like the CIs are not happy about the docs and loading the Matplotlib Qt backend fails with Python 3.7.

I can't check what's wrong with the docs it seems. Building locally, it complains about 'secondary_sidebar_items': ['page-toc'] added here.

adam2392 commented 1 year ago

It looks like the CIs are not happy about the docs and loading the Matplotlib Qt backend fails with Python 3.7.

I can't check what's wrong with the docs it seems. Building locally, it complains about 'secondary_sidebar_items': ['page-toc'] added here.

Should be fixed now on main.

adam2392 commented 1 year ago

Feel free to ping me whenever you need me to take a look again!

adam2392 commented 1 year ago

Happy new years @ruuskas ! Just wanted to check in on this. It would be great to get this in and make a new release w/ all your cool improvements!

ruuskas commented 1 year ago

Happy new year @adam2392 ! I have been focusing on other things so this has been lagging. I'll try to get the necessary changes done this week (maybe today if there are no blockers).

ruuskas commented 1 year ago

Hi @adam2392 ! This should now be ready for review again. The freqs parameter is now required and I added a test for padding. It was a good idea since there was a bug hiding indeed.

Sphinx is not happy as there seems to be an issue with indentation in the docstring, which I couldn't figure out without changing config.

drammock commented 1 year ago

Sphinx is not happy as there seems to be an issue with indentation in the docstring, which I couldn't figure out without changing config.

The sphinx error:

sphinx.errors.ThemeError: An error happened in rendering the page api.
Reason: UndefinedError("'logo' is undefined")

is due to a change in sphinx 6.0 and is currently being addressed upstream in the website theme. Temporary workaround is to pin sphinx to <6.0

ruuskas commented 1 year ago

The Sphinx error I get locally is different. I can copy it verbatim tomorrow, but essentially "unexpected indentation" in the docstring. Indentation looks fine where it points to though.

adam2392 commented 1 year ago

@ruuskas I fixed the indentation error and other sphinx issue. You have to git pull to sync changes.

ruuskas commented 1 year ago

Hi @adam2392! I addressed all the things you pointed out and pushed changes. WDYT?

adam2392 commented 1 year ago

Thanks @ruuskas! Great addition and contribution.

ruuskas commented 1 year ago

Thanks @adam2392!