-
Notifications
You must be signed in to change notification settings - Fork 7
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
[ENH] Add fastSRM functionality #70
Conversation
Opened hugorichard/FastSRM#6 for fastSRM package deprecations ! |
LMK when you want a review. |
@bthirion I think this is now ready ! I've kept the existing PR mostly as-is (just dealing with build and deprecation issues), so would be good to go over it. Will also review myself ! |
@thomasbazeille I'm guessing you don't have time / interest to review, but your input is also always welcome ! 🤗 |
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'm realizing that I'm confused. Are these modules dedicated to SRM specifically or can they be used with other estimators ?
fmralign/piecewise.py
Outdated
|
||
reshaped_aligned = | ||
|
||
!!!Not implemented for n_bags>1 |
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.
Should we worry about that ? Issue a warning ?
fmralign/piecewise.py
Outdated
|
||
Parameters | ||
---------- | ||
X_i: ndarray |
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.
The docstring does not march the signature here.
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.
Looks like this was pulled from the existing fit_one_piece
, which is maybe another plus for integrating it there. Noting that we'll also need to handle Identity
differently or we'll have overloaded params.
fmralign/piecewise.py
Outdated
|
||
def fit_one_parcellation( | ||
X_list, | ||
srm, |
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.
alignment_method ?
fmralign/piecewise.py
Outdated
|
||
def __init__( | ||
self, | ||
srm, |
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.
alignment_method ?
I believe these can only be used with SRM, since SRM is a group-level method so we have to handle training data differently. There is already a fit_one_piece method and (non-overlapping, but similarly named) generate_Xi_Yi functions in One option is to add another |
@bthirion I've gone through and patched up the obviously outdated docstrings ; I'm thinking to leave the reorg to another PR, though, just to get the initial code in. WDYT ? |
cc75363
to
0e1ce05
Compare
OK, I'm worried that this is now blocking #74 (as we patch dependencies for both nilearn and fastsrm here), so I'd like to get this merged in. Similar to the discussion there, this has SRM in a standalone module. Please let me know, @bthirion and @denisfouchard, if you have any concerns with merging this ! If I don't hear otherwise, I'll plan to merge early next week. |
LGTM. @denisfouchard please voice your opinion. |
No problems, as SRM is independent from my work. I presume that FastSRM being a more "reputable" package it is ok to keep it standalone. How about fugw btw ? Would it be nice to integrate it to compare to basic OT method ? Maybe @alexisthual ? |
Thanks, both ! I'm going to go ahead and merge this then, but I opened a new issue with your comment re : fugw so it's not lost ! We can continue the discussion there 🙂 |
Supersedes #50