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 RANSAC-breaking bug in RANSAC correlation window chunking code #67

Merged
merged 3 commits into from Apr 19, 2021

Conversation

a-hurst
Copy link
Collaborator

@a-hurst a-hurst commented Apr 18, 2021

PR Description

Partially addresses #41 and #65. Basically, the existing code incorrectly splits the full EEG data into window-sized chunks for RANSAC correlation. For convenience I'm copying my explanation/illustration from #65 below:

What we want it to do is take data like this (where A, B, and C are different channels):

[['A1', 'A2', 'A3', 'A4', 'A5', 'A6', 'A7', 'A8'],
 ['B1', 'B2', 'B3', 'B4', 'B5', 'B6', 'B7', 'B8'],
 ['C1', 'C2', 'C3', 'C4', 'C5', 'C6', 'C7', 'C8']]

... and reshape it into this (where the number of correlation windows is 4, and there are 2 samples per window):

[[['A1', 'A2'],
  ['B1', 'B2'],
  ['C1', 'C2']],

 [['A3', 'A4'],
  ['B3', 'B4'],
  ['C3', 'C4']],

 [['A5', 'A6'],
  ['B5', 'B6'],
  ['C5', 'C6']],

 [['A7', 'A8'],
  ['B7', 'B8'],
  ['C7', 'C8']]]

...such that when you iterate over the first axis, the windows of data look like the above chunks.

Instead, the PyPREP code above generates this:

[[['A1', 'A2', 'A3', 'A4'],
  ['A5', 'A6', 'A7', 'A8']],

 [['B1', 'B2', 'B3', 'B4'],
  ['B5', 'B6', 'B7', 'B8']],

 [['C1', 'C2', 'C3', 'C4'],
  ['C5', 'C6', 'C7', 'C8']]]

... which is then sliced along the last axis, resulting in windows of data like this:

[[['A1', 'A5'],
  ['B1', 'B5'],
  ['C1', 'C5']],

 [['A2', 'A6'],
  ['B2', 'B6'],
  ['C2', 'C6']],

 [['A3', 'A7'],
  ['B3', 'B7'],
  ['C3', 'C7']],

 [['A4', 'A8'],
  ['B4', 'B8'],
  ['C4', 'C8']]]

The current PR fixes things to work as intended.

Merge Checklist

  • the PR has been reviewed and all comments are resolved
  • all CI checks pass
  • (if applicable): the PR description includes the phrase closes #<issue-number> to automatically close an issue
  • (if applicable): bug fixes, new features, or API changes are documented in whats_new.rst

@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@032cad1). Click here to learn what that means.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #67   +/-   ##
=========================================
  Coverage          ?   97.00%           
=========================================
  Files             ?        7           
  Lines             ?      568           
  Branches          ?        0           
=========================================
  Hits              ?      551           
  Misses            ?       17           
  Partials          ?        0           
Impacted Files Coverage Δ
pyprep/ransac.py 97.24% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 032cad1...e82f111. Read the comment docs.

@a-hurst
Copy link
Collaborator Author

a-hurst commented Apr 18, 2021

What's absolutely wild to me is that the RANSAC unit tests still worked totally fine despite this bug. If you can think of an improved method of unit-testing RANSAC, I can add that to this PR request as well: otherwise, we can just wait for the testing-vs-MATLAB CI updates.

Copy link
Owner

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have an idea for a unit test right now ... currently I am fully relying on your local tests against Matlab PREP :)

it'd be great to get those tests online soon.

The best would of course be to do that directly in CI, as we discuss in #49

docs/whats_new.rst Outdated Show resolved Hide resolved
@sappelhoff sappelhoff merged commit cd7cb92 into sappelhoff:master Apr 19, 2021
@sappelhoff sappelhoff added this to the 0.3.2 milestone Apr 19, 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.

None yet

3 participants