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

Make PyPREP's random channel picks match MATLAB PREP's for the same random seeds #62

Merged
merged 9 commits into from Apr 10, 2021

Conversation

a-hurst
Copy link
Collaborator

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

PR Description

Closes #61. This PR does two main things:

  1. Re-implements MATLAB PREP's custom randsample function so that we can generate identical random channel picks using the same random seed
  2. Changes the RANSAC sample size from (0.25 * number of good channels) to (0.25 * number of all channels), matching MATLAB PREP and AutoReject's implementations.

I also adjusted the "too few channels" exceptions to be more informative, based on the second change. Let me know if you have any questions!

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

@sappelhoff sappelhoff added this to the 0.3.2 milestone Apr 10, 2021
@a-hurst a-hurst mentioned this pull request Apr 10, 2021
23 tasks
@a-hurst
Copy link
Collaborator Author

a-hurst commented Apr 10, 2021

Argh, I keep accidentally making style mistakes. Gotta fix VSCode's linting setup to automatically catch this stuff...

@a-hurst a-hurst requested a review from sappelhoff April 10, 2021 15:48
@a-hurst
Copy link
Collaborator Author

a-hurst commented Apr 10, 2021

@sappelhoff Okay, finally fixed the unit tests! Apparently bad-by-flat channels get removed entirely from the data during NoisyChannels, so I wasn't triggering the 'num_good_chans' < 'ransac_sample_size' like I thought I would. Double-checked and MATLAB PREP does the same thing, so no additional changes needed to match it.

@a-hurst
Copy link
Collaborator Author

a-hurst commented Apr 10, 2021

Made one additional change to better match MATLAB PREP: instead of rounding (0.25 * number of channels) up, they round it to the nearest integer, so I tweaked PyPREP to do the same. Here's the relevant MatPREP code:

ransacSubset = round(noisyOut.ransacChannelFraction*size(data, 2));

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.

LGTM, thanks :)

@sappelhoff sappelhoff merged commit a01902e into sappelhoff:master Apr 10, 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.

RANSAC channel picks differ between MATLAB PREP and PyPREP using the same seed
2 participants