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

Improvements to Granger Causality evaluation #428

Closed
wants to merge 9 commits into from

Conversation

DavideNuzzi
Copy link

Modified bst_granger.m and bst_granger_spectral.m (their results were not correct)
Implemented the conditional GC both in time and frequency domain.
The functions "YuleWalker_Inverse" and "YuleWalker_Mask" are only used for the solution (maybe they can be moved in another folder).

Note that the approach I choose to follow in order to evaluate GCs can be readily generalized. For example it will be possible to evaluate multivariate GC (multiple source variables) both in time and frequency domain.

Modified bst_granger.m and bst_granger_spectral.m (their results were not correct)
Implemented the conditional GC both in time and frequency domain.
@ftadel
Copy link
Member

ftadel commented Jul 26, 2021

Thank you for these propositions. This represents a lot of work, and will require a lot of time to review on our end.
There are two distinct objectives to this PR, which need to be reviewed/approved separately:

  • the modification of the existing Granger causality code
  • the inclusion of new methods, which is far from being finished, as it would require the corresponding process functions and more in-depth discussions about their necessity with the PIs of the project.

@DavideNuzzi

  • Please split this PR in two different PRs, to address each item separately.
  • Remove capital letters from any new file names
  • Include the small sub-functions in existing functions or "private" subfolders (we don't want to add too many .m files that are called in only one or two places to be added to the Matlab path and the general workspace)
  • Please be patient, as you may not get any additional feedback before mid-August

@HosseinShahabi @richardmleahy @rcassani @sbaillet
Could you please share your first impressions about these suggested changes?

Thanks

@danielemarinazzo
Copy link
Contributor

dear team @HosseinShahabi @sbaillet @richardmleahy @rcassani @ftadel
this PR is related to #419 . I have gone through the code with @DavideNuzzi

@DavideNuzzi
Copy link
Author

DavideNuzzi commented Jul 30, 2021

Regarding the sub-functions YuleWalker_Mask.m and YuleWalker_Inverse.m, they are called by each of the GC functions that I've written (pairwise and conditional GC in both time and frequency domain). That's the reason why I preferred to define them in separate .m files instead of duplicating them in the main functions. Do you suggest I do it anyway?

If you'd prefer to keep them as separate functions, please:

  • remove the capital letters (eg. yule_walker_mask.m or yulewalker_mask.m)
  • move them to the folder brainstorm3/toolbox/connectivity/private so that they don't end up in the global Matlab namespace

Thanks

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

Successfully merging this pull request may close these issues.

3 participants