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

MIssing plots in granger_fmri.html #173

Closed
justbennet opened this issue Jun 24, 2019 · 14 comments · Fixed by #176
Closed

MIssing plots in granger_fmri.html #173

justbennet opened this issue Jun 24, 2019 · 14 comments · Fixed by #176

Comments

@justbennet
Copy link

There might be something not right about the last two figures here

http://nipy.org/nitime/examples/granger_fmri.html

Those are missing the body of the graph, which is all white.

I believe this is in the file doc/examples/granger_fmri.py.

@arokem
Copy link
Member

arokem commented Jun 24, 2019

Thanks for spotting this. Looks like a lot of nan's in the results from Granger Causality calculation. Since it's been a while since the docs were built, I'll probably need to bisect back in history to see where this happened. Unfortunately, it might take me a little while to get to it.

@arokem arokem mentioned this issue Jun 25, 2019
@justbennet
Copy link
Author

justbennet commented Jul 4, 2019 via email

@arokem
Copy link
Member

arokem commented Jul 9, 2019 via email

@justbennet
Copy link
Author

Thanks. Just curious because the issue got closed but the problem seems to remain...? There is also a question on the Neuroimaging mailing list about it,

Subject: [Neuroimaging] GC analysis with nitime
Date: Fri, Jul 5, 2019 at 7:37 PM

Should the issue be reopened until it gets addressed?

@arokem
Copy link
Member

arokem commented Jul 10, 2019

Clearly.

@arokem arokem reopened this Jul 10, 2019
@hvannieuwenh
Copy link

Hi, I had the same issue when trying to run the Granger causality example for fMRI on my own computer. I tried rolling back to older versions of nitime (0.7, 0.6, and 0.5) but they had their own problems with dependent packages. Is there any way on my end to make the Granger causality module functional? It is exactly the tool I am looking for in my research. Thanks.

@Xunius
Copy link
Contributor

Xunius commented Sep 26, 2021

Hi all,
I spotted the same issue. Although being totally ignorant about the inner maths about Granger, I think I figured out where the problem lies.

The GrangerAnalyzer puts its outputs in the upper triangle:

        if ij is None:
            # The following gets the full list of combinations of
            # non-same i's and j's:
            x, y = np.meshgrid(np.arange(self._n_process),
                               np.arange(self._n_process))
            self.ij = list(zip(x[tril_indices_from(x, -1)],
                          y[tril_indices_from(y, -1)]))

Note the order is x then y, so self.ij indexes into the upper triangle.

While in making the plot, drawmatrix_channels() leaves only the lower triangle:

    # Null the upper triangle, so that you don't get the redundant and the
    # diagonal values:
    idx_null = triu_indices(m.shape[0])
    m[idx_null] = np.nan

So we ended up with nothing.

I suppose the fix is to transpose the ordering in GrangerAnalyzer like so:

           self.ij = list(zip(y[tril_indices_from(y, -1)],
                          x[tril_indices_from(x, -1)]))

This gives the following image for the fig03 in the tutorial:

Figure_3

fig04 is like this:

Figure_4

But, I'm not familiar with the plotting conventions people use in such like of analyses. Do we assume the plot in fig03 is the "causality" from row to column, or the other way around? Please help verity the proposed fix.

@arokem
Copy link
Member

arokem commented Sep 26, 2021

Thanks for figuring this out! I think that the best fix would probably be to alter the behavior of drawmatrix_channels. Do you want to try to make a PR for that?

@Xunius
Copy link
Contributor

Xunius commented Sep 26, 2021

@arokem I'm not sure what exact changes to make to drawmatrix_channels(), because it's working fine for other plots and only fails for Granger. How to change it so we don't break compatibility with other tutorials and with older versions?

@arokem
Copy link
Member

arokem commented Sep 27, 2021

Gotcha. Yeah - I think that your original solution makes more sense then. Arguably, this is the design we should have implemented to begin with. Could you make a PR with that implemeted?

@Xunius
Copy link
Contributor

Xunius commented Sep 27, 2021

@arokem It seems that the fix is failing the test_granger.py test. Now I actually think that proposed fix is not ideal, it would break with old behavior. We need to be more careful.

@arokem
Copy link
Member

arokem commented Sep 27, 2021

Yes - we need to adjust the test and warn about the break to backwards compatibility.

If you reopen the PR, we can discuss that there.

@Xunius
Copy link
Contributor

Xunius commented Sep 28, 2021

I made some back-n-forth changes, hopefully not creating too much confusion to the git history.
In the latest PR I changed the drawmatrix_channels() instead to use the upper triangle but transpose it to lower before plotting. This doesn't affect other tutorials because coherence, correlation etc. are all symmetrical. I've tested all scripts in examples that calls drawmatrix_channels() and the figures all look good.

@arokem
Copy link
Member

arokem commented Sep 28, 2021

Closed through #193

@arokem arokem closed this as completed Sep 28, 2021
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 a pull request may close this issue.

4 participants