Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MAINT] Refactor and move computation functions for epochs and time in spectral submodule to a sep file #127

Closed
adam2392 opened this issue Feb 22, 2023 · 11 comments

Comments

@adam2392
Copy link
Member

Discussed in call w/ Thomas Binns that I will move refactor and clean up the epochs.py and time.py files inside spectral submodule to make the code cleaner to read

@adam2392
Copy link
Member Author

@tsbinns is this accurate?

@tsbinns
Copy link
Collaborator

tsbinns commented Feb 23, 2023

Yes, thank you @adam2392!

@tsbinns
Copy link
Collaborator

tsbinns commented Nov 6, 2023

@adam2392 Would this be the right place to discuss the other refactoring points raised by @drammock in #142, or would a new issue be better?

@adam2392
Copy link
Member Author

adam2392 commented Nov 6, 2023

Sure we can discuss the refactoring of the files here.

@larsoner
Copy link
Member

larsoner commented Nov 6, 2023

+1 for a new PR that just copy-pastes code and adds a few import statements to glue everything together

@tsbinns
Copy link
Collaborator

tsbinns commented Nov 6, 2023

+1 for a new PR that just copy-pastes code and adds a few import statements to glue everything together

So something along the lines of:

  • epochs.py handling the code required for both bivariate and multivariate methods
  • epochs_bivariate.py handling the code specific for the bivariate methods
  • epochs_multivariate.py handling the code specific for the multivariate methods

Then a similar thing for spectral_connectivity_time.

Would this be okay?

@adam2392
Copy link
Member Author

adam2392 commented Nov 6, 2023

That seems reasonable to me

@adam2392
Copy link
Member Author

adam2392 commented Nov 6, 2023

@tsbinns thanks for all the hard work in working w/ us on getting the multivariate methods into proper shape and also assisting w/ the refactoring.

FYI I sent an invite to collaborate on the mne-connectivity repo, so you can have better control over GH issues

@tsbinns
Copy link
Collaborator

tsbinns commented Nov 6, 2023

Thanks very much @adam2392, happy to help!

epochs.py handling the code required for both bivariate and multivariate methods
epochs_bivariate.py handling the code specific for the bivariate methods
epochs_multivariate.py handling the code specific for the multivariate methods

From the looks of things, people are happy with this approach. The next days are pretty busy for me, but I will try to have something for later next week.

@tsbinns
Copy link
Collaborator

tsbinns commented Nov 28, 2023

The changes discussed here were merged in #156.

@adam2392 Do you think there is anything more that needs to be done for this issue?

@adam2392
Copy link
Member Author

Yes we can close this now! Thanks!

@tsbinns tsbinns closed this as completed Nov 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants