srm: subtract the subject-specific mean in .transform()

[qihongl]

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.

[qihongl]

Sorry, I didn't expected this to trigger some run test failures and I will take a look later...
btw, I can't view the linux and macos error. I got a message saying "qlu is missing the Overall/Read permission". Should I be able to look at the report?

[qihongl]

Based on the travis report, the failure came from the tests for sssrm. It seems that sssrm is using (and only using) the _init_w_transforms function from srm, but the error occured when calling the fit function of sssrm. So I'm a little confused about why changing the transform function from srm would break that, since srm's transform doesn't call srm's _init_w_transforms... Any clue?

[mihaic]

See PR #408.

[TuKo]

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.

[qihongl]

@TuKo Ah I see. transform_subject() is solving a standard procrustes problem. Is there a direct extension for procrustes with an intercept? Or does it make sense just do some mean centering, align by standard procrustes, and add the mean back? I'm frankly not sure what's the best thing to do here.

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 example/.

[qihongl]

@TuKo I had a conversation with Sam and I guess for the transform_subject() function with intercept (i.e. mu), we probably want to do some mean centering for the estimated S and X_new, then align, and add the mean of S back. In this case, I was wondering if the user should do this manually? Does it seem strange to do this for the user implicitly?

[TuKo]

@qihongl transform_subject() should compute the mean and substract it from data to compute the new subject's transform (see here ). BTW, the mu obtained for the new subject should be returned from the function as well.

Please note that DetSRM, RSRM and SS-SRM should follow SRM by removing mean.
I was suggesting to document the other classes, but you are welcome to modify them too. :)

[qihongl]

I see! Thanks! @TuKo

[mihaic]

@qihongl, any progress?

[mihaic]

@qihongl, are you planning to finish this PR?

[qihongl]

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!

[qihongl]

Sorry I finally got to this... Can I make a new clone of the latest brainiak and start this from scratch?

[mihaic]

If that works better for you, go ahead, @qihongl.

[qihongl]

sounds great!

[qihongl]

@mihaic
I want to update the documentation for DetSRM, RSRM and SS-SRM to mention that they don't apply mean removal as SRM. Should I just add this comment in the class doc string? Thanks!

[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.