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
srm: subtract the subject-specific mean in srm.transform() and srm.transform_subject() #495
base: master
Are you sure you want to change the base?
Conversation
Hi @mihaic I made a new PR about SRM intercept here. |
I think that this is still a problem in the other SRM classes. Namely, DetSRM should handle intercept/bias vectors similarly as SRM. |
Other than that, the change for |
Thanks! @TuKo But does |
btw I think we should recommend people normalizing the data before using any SRM. Assuming the data is standardized, all SRMs can work well even without adjusting for subject-specific intercept terms. Of course, normalization also handles subject-specific scaling, which is also important. Fortunately, this seems to be what people have been doing. |
No, it doesn't. But it should have that option, if we are going to assume that data is not z-scored. |
I see. I just modified detsrm to allow it to handle intercepts. I also added a test to show that now detSRM can align subjects with different intercept terms (the same as the SRM intercept test). |
…h should save some memory...
…g 1 return var, but now it return both w and mu
There is another failed test that I don't understand. @TuKo do you know why it is failing? |
The unit tests are passing. The documentation building is failing, but I can fix that, don't worry about it.
https://travis-ci.org/github/brainiak/brainiak/jobs/767874844 |
Sounds great! Thanks! @mihaic |
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.
All the code looks good. One small discrepancy between changes and code/documentation is left. Once it is corrected it should be ready to go.
# Solve the Procrustes problem | ||
U, _, V = np.linalg.svd(A, full_matrices=False) | ||
return U.dot(V) | ||
return U.dot(V), mu | ||
|
||
def transform_subject(self, X): | ||
"""Transform a new subject using the existing model. |
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.
As the function _update_transform_subject()
was modified to return mu
, then we should update the documentation or code of transform_subject()
.
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.
thanks! what do you mean? I did update the doc string of transform_subject()
about mu
This is a continuation of #406
I updated SRM to use the intercept term. Then in the srm test, I added an example showing that ...
X_i = W_i.T @ S + intercept_i
Then SRM can align them (only possible with the intercept term)
X_new = W_new.T @ S + intercept_new
Then srm.transform_subject() can align it to a trained shared response. It returns the estimated w and intercept (called
w
andmu
). A short comment about how to usew
andmu
is added to the doc string.I also added comments in detsrm, sssrm, rsrm about they current don't use the intercept term.
The auto-styling functionality of my editor added some spaces here and there, which shouldn't impact the functionality of the code.