-
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
Add PiecewiseSRM to the package #49
Comments
I'd rather import it, but you seem to be reluctant to that, and I'm not sure why ? |
I guess it is mostly a question of future maintenance and involvement. Having a strong dependency on a personal package seems like something that's likely to make things break if it is not maintained and bringing everything in the same place also means centralizing future maintenance but I don't have a strong opinion. |
Personal +1 that a central location might decentralize maintenance, but @hugorichard 's opinion is probably the most pressing, here ! |
Maybe my bad wording in the first place introduced a misunderstanding. If by
you mean we should have a copy of one SRM inside fmralign, then I think we all agree. |
I would rather import it. FastSRM is already used in Multiview ICA. Maybe I could remove this dependency though. I need to think about it. |
+1 for import (sorry if I was unclear) |
@hugorichard did you have any time to think about this ? |
Should I include our implementation here as a PR ? |
Yes, please add a PR that imports FastSRM properly. Thx ! |
@thomasbazeille @bthirion I have added a PR #50 . CircleCI seem to be broken (the tests do not even run). |
Goal : Piecewise SRM
Main questions :
Other than that, main depencies are compatible and fmralign helpers are already used. A few functions are duplicated to handle the list input (instead of pairwise input usually) such as
generate_X_is
,fit_one_piece
,fit_one_parcellation
. We could either refactor their counterpart in fmralign, or keep the duplication in order to keep a clearer codebase.The text was updated successfully, but these errors were encountered: