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

Numeric equivalence with MATLAB PREP #58

Closed
22 of 23 tasks
a-hurst opened this issue Apr 8, 2021 · 12 comments
Closed
22 of 23 tasks

Numeric equivalence with MATLAB PREP #58

a-hurst opened this issue Apr 8, 2021 · 12 comments
Milestone

Comments

@a-hurst
Copy link
Collaborator

a-hurst commented Apr 8, 2021

This is a general tracking issue for the numeric equivalence of PyPREP vs Matlab PREP at each stage of the pipeline. I figure this will be useful for keeping a bird's eye view of what's been tested and what hasn't, as well as what parts have been confirmed to differ.

Guide: items with a checkmark have been tested and confirmed as producing identical results to MATLAB. Items with no checkmark and a corresponding issue # have been tested and confirmed as diverging. Items with no checkmark and no issue number have yet to be formally tested.

NoisyChannels Methods

Other Functions

@sappelhoff sappelhoff added this to the 0.3.2 milestone Apr 8, 2021
@sappelhoff
Copy link
Owner

Hey @a-hurst this is amazing, you are doing a lot of great work for pyprep, thanks :-) I sent you a "collaborator" invitation for this GitHub repo, which will give you a couple of more permissions that will hopefully be helpful for organizing labels, milestones, reviews, etc.

@a-hurst
Copy link
Collaborator Author

a-hurst commented Apr 8, 2021

@sappelhoff Glad I can help! I've definitely learned a lot through the process so far. Thanks for the collaborator invite, and for your helpful feedback on my posts and PRs!

@sappelhoff sappelhoff pinned this issue Apr 10, 2021
@yjmantilla yjmantilla unpinned this issue Apr 12, 2021
@a-hurst
Copy link
Collaborator Author

a-hurst commented Apr 12, 2021

@sappelhoff @yjmantilla do either of you have any data with channel drop-outs, just so I can test that and (hopefully) check it off? Or would that be really easy to simulate for a given recording? As an EEG novice (my background is almost entirely behavioural psych), I'm assuming that dropouts are what happens when the amplifier loses connection with an electrode for some reason, but I'm not sure how that ends up looking in the data.

@sappelhoff
Copy link
Owner

do either of you have any data with channel drop-outs, just so I can test that and (hopefully) check it off?

Mh I don't have such data at hand right now (nothing comes to mind), but for the data that I worked with (BrainVision ".vhdr+.vmrk+.eeg" format), I never encountered NaN values during recording or analysis.

I did have some recordings where an electrode got loose and then connection was bad ... and other cases where an electrode itself was broken.

That usually resulted in very flat (very low variance and amplitude) signals.

@a-hurst
Copy link
Collaborator Author

a-hurst commented Apr 12, 2021

but for the data that I worked with (BrainVision ".vhdr+.vmrk+.eeg" format), I never encountered NaN values during recording or analysis.

I don't think EDF/BDF files have any concept of NaN either, since all data is stored exclusively as integers. Maybe EEGLAB generates NaNs under certain circumstances that MNE doesn't? If that's the case, maybe for PyPREP dropouts should be based on windows where the signal is totally flat instead of windows where the signal is NaN. The original PREP publication reports a dataset they used where they had considerable dropouts, so maybe I should fire them an email...

@sappelhoff
Copy link
Owner

Once numerical equivalence with matlab PREP is reached, we can consider removing the "ALPHA SOFTWARE" warning from the README:

ALPHA SOFTWARE. This package is currently in its early stages of iteration. It may change both its internals or its user-facing API in the near future. Any feedback and ideas on how to improve either of these is more than welcome! Use this software at your own risk.

@yjmantilla
Copy link
Collaborator

Seems I unpinned this issue without me noticing (maybe I miss-clicked somewhere (?)). Pinning it back.

@a-hurst
Copy link
Collaborator Author

a-hurst commented Jun 28, 2021

@sappelhoff @yjmantilla With the merging of #96, I think we've identified and resolved every deviation between PyPREP and MatPREP apart from the adaptive line noise stuff 🎉🎊🎉. Massive thanks to you both for your help, feedback, and support in my crusade against issue #41 and beyond: I feel like I've grown a lot as a programmer from the collaborative atmosphere and the detailed code review I've received contributing to this project.

Anyway, since the CleanLine stuff has been bumped to 0.5.0, should this issue get bumped to 0.5.0 as well, or should it be closed in favour of #82 since it's the only component remaining?

@sappelhoff
Copy link
Owner

🎉 🚀 awesome! :-) You are doing a tremendous job and it's a pleasure to review your PRs - always cleanly documented, well described, and effective. I am happy you've taken something beyond the (already awesome) improvement of PyPrep (I also did / and do in my role as maintainer/reviewer).

I think this issue can be closed with its last box (cleanline) unticked until we close #82. I'll leave the closing and unpinning of this one (#58) to you :-) cheers!

PS: Can't wait to finally ship all the good stuff in 0.4

@yjmantilla
Copy link
Collaborator

yjmantilla commented Jun 28, 2021

Also very happy! I haven't really contributed to pyprep these last months but I feel like-wise as @a-hurst said:

I feel like I've grown a lot as a programmer from the collaborative atmosphere and the detailed code review I've received contributing to this project.

So a lot of thanks to @sappelhoff for the patience and reviews too. All the stuff he has taught me has improved my programming as an undergraduate, and I have tried to replicate those patterns here with my colleagues.

@a-hurst
Copy link
Collaborator Author

a-hurst commented Jun 28, 2021

Thanks! 🙂 And yeah, a lot of my open-source experience has been contributing to semi-abandoned projects that I've tried to keep alive (specifically eyelinker and pysdl2), so outside of PyPREP I haven't had many opportunities to do real collaborative software work. It's been a nice change of pace! A lot easier to stay motivated to see something like this through when you know at least one or two other people are going to notice and appreciate it 😊

Anyway, closing this via #96!

@a-hurst a-hurst closed this as completed Jun 28, 2021
@a-hurst a-hurst unpinned this issue Jun 28, 2021
@sappelhoff
Copy link
Owner

Thanks @yjmantilla I am glad the reviews helped you :)

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

No branches or pull requests

3 participants