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

fix all nans in GrangerAnalyzer #193

Merged
merged 3 commits into from
Sep 28, 2021
Merged

fix all nans in GrangerAnalyzer #193

merged 3 commits into from
Sep 28, 2021

Conversation

Xunius
Copy link
Contributor

@Xunius Xunius commented Sep 27, 2021

This is to fix the issue #173.

GrangerAnalyzer put its outputs in the upper triangle only, which
would be removed by drawmatrix_channels() which takes only the lower
triangle, creating an array of all nans.

This fix changes the drawmatrix_channels() to let it take the upper
triangle instead, then transpose the matrix before plotting.

Another possible fix is to transposes outputs of GrangerAnalyzer into the
lower triangle. However this would break with the old behavior including
failing unit tests (test_granger.py).

Another reason to change the drawmatrix_channels() instead is that
other similar functions, including CorrelationAnalyzer.xcorr,
CoherenceAnalyzer.coherence all computes the upper triangle first,
then transpose to fill the lower, e.g.

idx = tril_indices(tseries_length, -1)
xcorr[idx[0], idx[1], ...] = xcorr[idx[1], idx[0], ...]

But granger is a bit different in that it is not symmetrical so we don't
just mirror the upper triangle into the low.

Another reason to keep the upper triangle in granger is that the
indices of pairs argument (ij) of the GrangerAnalyzer.__init__()
translates more naturally to the upper triangle indices, e.g. pair
(0, 1) -> (row-0, column-1),
(2, 4) -> (row-2, column-4).
If feels more intuitive (to me at least) to preserve this, rather than doing a
transpose here. This also helps pass the test_granger.py unit test.

Related changes needed:

in doc/examples/multi_taper_coh.py, I have to change the nested for loop
so the results are put into the upper triangle. The old for loop:

for i in range(nseq):
    for j in range(i):

New:

for i in range(nseq):
    for j in range(i, nseq):

This is also consistent with CoherenceAnalyzer etc..

I've tested all scripts in the doc/examples folder that involve the
drawmatrix_channels() function. All figures look good.

This is to fix issue #173 (comment).

`GrangerAnalyzer` put its outputs in the upper triangle only, which
would be removed by `drawmatrix_channels()` which takes only the lower
triangle, creating an array of all `nan`s.

This fix transposes outputs of `GrangerAnalyzer` into the lower
triangle. This also implies that the element (i,j) in the `causality_xy`
attribute denotes "causality" from row-i to column-j.
@Xunius Xunius closed this Sep 27, 2021
This is to fix the issue #173.

`GrangerAnalyzer` put its outputs in the upper triangle only, which
would be removed by `drawmatrix_channels()` which takes only the lower
triangle, creating an array of all nans.

This fix changes the `drawmatrix_channels()` to let it take the upper
triangle instead, then transpose the matrix before plotting.

Another possible fix is to transposes outputs of `GrangerAnalyzer` into the
lower triangle. However this would break with the old behavior including
failing unit tests (`test_granger.py`).

Another reason to change the `drawmatrix_channels()` instead is that
other similar functions, including `CorrelationAnalyzer.xcorr`,
`CoherenceAnalyzer.coherence` all computes the upper triangle first,
then transpose to fill the lower, e.g.

```
idx = tril_indices(tseries_length, -1)
xcorr[idx[0], idx[1], ...] = xcorr[idx[1], idx[0], ...]
```

But granger is a bit different in that it is not symmetrical so we don't
just mirror the upper triangle into the low.

Another reason to keep the upper triangle in granger is that the
indices of pairs argument (`ij`) of the `GrangerAnalyzer.__init__()`
translates more naturally to the upper triangle indices, e.g. pair (0,
1) -> (row-0, column-1), (2, 4) -> (row-2, column-4). If feels more
intuitive (to me at least) to preserve this, rather than doing a
transpose here. This also passes the `test_granger.py` unit test.

Related changes needed:

in `doc/examples/multi_taper_coh.py`, I have to change the nested `for` loop
so the results are put into the upper triangle. The old `for` loop:

```
for i in range(nseq):
    for j in range(i):
```

New:

```
for i in range(nseq):
    for j in range(i, nseq):
```

This is also consistent with `CoherenceAnalyzer` etc..

I've tested all scripts in the `doc/examples` folder that involve the
`drawmatrix_channels()` function. All figures look good.
@Xunius Xunius reopened this Sep 28, 2021
@Xunius Xunius closed this Sep 28, 2021
@Xunius Xunius reopened this Sep 28, 2021
@Xunius Xunius changed the title fix all nans in GrangerAnalyzer by transposing into lower triangle fix all nans in GrangerAnalyzer Sep 28, 2021
@arokem
Copy link
Member

arokem commented Sep 28, 2021

This all makes sense to me!

@arokem arokem merged commit 8b83797 into nipy:master 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 this pull request may close these issues.

2 participants