srm: subtract the subject-specific mean in .transform()#407
srm: subtract the subject-specific mean in .transform()#407qihongl wants to merge 2 commits intobrainiak:masterfrom
Conversation
|
Sorry, I didn't expected this to trigger some run test failures and I will take a look later... |
|
Based on the travis report, the failure came from the tests for |
|
See PR #408. |
TuKo
left a comment
There was a problem hiding this comment.
The change in transform() looks correct.
However, the same bug should be corrected for computing a new subject in the function in
Line 384.
In addition, as the class actually differs from others, please update the documentation for DetSRM, RSRM and SS-SRM mentioning that each of these algorithms don't apply mean removal as SRM.
|
@TuKo Ah I see. Right, the documentation should be updated. And later on I can modify my notebook to show detSRM doesn't align two trajectories differ by a vector shift whereas SRM does, and upload it to |
|
@TuKo I had a conversation with Sam and I guess for the transform_subject() function with intercept (i.e. |
|
@qihongl Please note that DetSRM, RSRM and SS-SRM should follow SRM by removing mean. |
|
I see! Thanks! @TuKo |
|
@qihongl, any progress? |
|
@qihongl, are you planning to finish this PR? |
|
Ah my github is not linked to my main email address so I didn't notice this. And I totally forgot this PR. Right I should finish this PR -- I will work on it get back to you guys in 2 weeks! |
|
Sorry I finally got to this... Can I make a new clone of the latest brainiak and start this from scratch? |
|
If that works better for you, go ahead, @qihongl. |
|
sounds great! |
|
@mihaic |
|
Yes, the class docstring is a good spot. Of course, it would be even better to change the other code as well. Up to you. |
This is a fix described in #406 . With @cameronphchen 's fix, SRM can align two trajectories that differ by a vector shift by using the estimated
mu.