Fix handling of channels with dropouts (intermittent flat regions) within NoisyChannels #81
Conversation
|
Ah whoops, I accidentally based this off the MATLAB comparison test branch. Rebasing on master now. |
| with np.errstate(invalid='ignore'): # suppress divide-by-zero warnings | ||
| window_correlation = np.corrcoef(np.transpose(eeg_portion)) |
There was a problem hiding this comment.
in which cases would the np.errstate(invalid='ignore') be necessary? And could it results in window_correlation being NaN?
There was a problem hiding this comment.
Yep, inside np.corrcoef it uses the standard deviation of the inputs at some point in the calculation, and since for a dropout channel the SD is going to be 0, we get NaNs because of the division-by-zero error (and a corresponding RuntimeWarning message). However, the current code uses those NaNs to determine which channels are dropout channels for each window so things would need to be refactored a fair bit to prevent the NaNs in the first place.
I'm thinking of doing a proper refactor of NoisyChannels at some point once MATLAB comparison is merged, so I went with the path of least resistance for now.
There was a problem hiding this comment.
To clarify, the with np.errstate is just to suppress some division-by-zero runtime warnings that are currently inevitable/expected based on how the code is structured.
There was a problem hiding this comment.
that makes sense, thanks for clarifying.
Codecov Report
@@ Coverage Diff @@
## master #81 +/- ##
==========================================
+ Coverage 97.49% 97.66% +0.17%
==========================================
Files 7 7
Lines 678 686 +8
==========================================
+ Hits 661 670 +9
+ Misses 17 16 -1
Continue to review full report at Codecov.
|
PR Description
Closes #80. Because Numpy's quantile function handles NaN values differently than MATLAB, finding bad-by-correlation channels in
NoisyChannelsbroke completely if there were any correlation windows where a channel had a flat signal. Also, the code that was meant to replace NaN correlation values with zeros was accidentally doing so in the wrong matrix. Both of these are fixed by this PR!Merge Checklist
closes #<issue-number>to automatically close an issue