-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
[MRG] Improve limo doc and regression example #6783
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #6783 +/- ##
==========================================
+ Coverage 89.67% 89.69% +0.01%
==========================================
Files 430 430
Lines 77005 76973 -32
Branches 12546 12527 -19
==========================================
- Hits 69058 69042 -16
+ Misses 5140 5134 -6
+ Partials 2807 2797 -10 |
|
Need to fix merge conflicts ... |
|
It looks like the only things that failed on the first run were pep violations: Should be easy to fix and merge, I think you should do it ASAP @JoseAlanis |
|
oops, sorry, I thought my editor would automatically strip the trailing spaces on save. Thanks @jona-sassenhagen! |
|
Hey guys, I'm sorry, I messed up my branch while trying to fix some merge conflicts. @larsoner @jona-sassenhagen, should I reset to my previous commit and force push? or what's the best solution here. |
|
I would:
Then push force |
add sklearn regression add class linear regression update limo doc fix limo fetcher docstring fix merging issues
e0dddfc to
a348c79
Compare
|
thanks @larsoner, looks like it worked :) |
0f2cae0 to
6cb374e
Compare
|
@JoseAlanis why is this marked WIP, is there more to do from your end? If you think it's ready for review/merge you can set the title to MRG, otherwise people will assume you're still looking at it and probably not review. I've pushed a few small commits to fix small problems. @jona-sassenhagen can you take a look and let us know if you're happy? |
|
alright, thanks @larsoner, no, I think we're good to go for now. I'll try to be more careful with these type of issues next time. |
jona-sassenhagen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few stylistic comments.
add jonas suggestions Co-Authored-By: jona-sassenhagen <jona.sassenhagen@gmail.com>
|
@drammock can you review and merge if you're happy? |
|
I'll make a PR into @JoseAlanis's branch with some edits to the flow of the prose in the example. Separate from that, there's this issue: So you'll need to add |
drammock
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made a PR into your branch with some mild-to-moderate changes to the prose and the code. In addition to that, there are two larger issues (commented on here) that need to be addressed before this is ready to merge.
|
@drammock ready for review. |
|
@drammock ok for you? |
drammock
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, here are a bunch of suggested changes. Most are minor wording clarifications or code simplifications. Assuming CIs still come back green after incorporating these, I'm +1 for merge.
Co-Authored-By: Daniel McCloy <dan.mccloy@gmail.com>
thanks @drammock, I really appreaciate the comments and sugesstions. |
|
here's the rendered example, looks good: |
|
thanks @JoseAlanis ! |
|
Thanks for extensive reviews @drammock |
|
Thanks everybody! |
Reference issue
Example: Picks up on #6710.
What does this implement/fix?
Additional information
Hey guys, sorry for the long description.
As suggested in #6710, I'm adding a bunch of changes to improve the documentation in the LIMO dataset.
I also extended the corresponding example, now taking a deeper look at how the data looks like and what information is provided in the metadata of the LIMO epochs.
The second part of the example (~ line 255) picks up on some of the work I did during my GSoC project on improving linear regression functionality (also refer to #6710 for details).
One of the major changes was to use sklearn to fit the linear models, wich would provide some extra degree of flexibility when handling the output of the linear models. I included a proposal for how to wrap sklearn's LinearRegression in a class (~ 300) line, which allows to keep the results of the first level (i.e., subject level) analysis all together. Then we could use this class to compute specific outputs. Currently for instance, one can extract an evoked object containing the beta coefficients of the subject level analysis or one can extract the "raw" beta coefficients as an array, which could be later used for group-level analysis. Getting the "raw" beta values would also allow us to compute inference measures on a group-level or a subjects levels (e.g., r-squared to find best fitting electrodes for subjects, etc.).
For convenience, I define the linear regression class in the example.
Looking forward to hear your comments (cc @dengemann, @jona-sassenhagen).