Skip to content
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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

qihongl
Copy link
Member

@qihongl qihongl commented Apr 10, 2021

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

  1. if I have a bunch of subjects in the form of

X_i = W_i.T @ S + intercept_i

Then SRM can align them (only possible with the intercept term)

  1. After fitting, if I have a new subject in the form of

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 and mu). A short comment about how to use w and mu 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.

@qihongl
Copy link
Member Author

qihongl commented Apr 10, 2021

Hi @mihaic I made a new PR about SRM intercept here.

@mihaic mihaic linked an issue Apr 12, 2021 that may be closed by this pull request
@TuKo
Copy link
Contributor

TuKo commented Apr 13, 2021

I think that this is still a problem in the other SRM classes.
Instead of documenting the differences, why don't we just do the respective modifications (at least in DetSRM).

Namely, DetSRM should handle intercept/bias vectors similarly as SRM.

@TuKo
Copy link
Contributor

TuKo commented Apr 13, 2021

Other than that, the change for SRM looks good to me!

@qihongl
Copy link
Member Author

qihongl commented Apr 14, 2021

Thanks! @TuKo But does detSRM estimate mu during model fitting? I thought it is just estimating n_units x n_units orthogonal transformations.

@qihongl
Copy link
Member Author

qihongl commented Apr 15, 2021

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.

@TuKo
Copy link
Contributor

TuKo commented Apr 20, 2021

Thanks! @TuKo But does detSRM estimate mu during model fitting? I thought it is just estimating n_units x n_units orthogonal transformations.

No, it doesn't. But it should have that option, if we are going to assume that data is not z-scored.

@qihongl
Copy link
Member Author

qihongl commented Apr 21, 2021

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

@qihongl
Copy link
Member Author

qihongl commented Apr 21, 2021

There is another failed test that I don't understand. @TuKo do you know why it is failing?

@mihaic
Copy link
Member

mihaic commented Apr 21, 2021

The unit tests are passing. The documentation building is failing, but I can fix that, don't worry about it.

================ 199 passed, 1394 warnings in 527.82s (0:08:47) ================

https://travis-ci.org/github/brainiak/brainiak/jobs/767874844

@qihongl
Copy link
Member Author

qihongl commented Apr 21, 2021

Sounds great! Thanks! @mihaic

@qihongl
Copy link
Member Author

qihongl commented Sep 19, 2021

Hi @mihaic @TuKo it just occurred to me -- is there anything else I can help with for this PR? Thanks!

Copy link
Contributor

@TuKo TuKo left a 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.
Copy link
Contributor

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().

Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SRM doesn't apply mu in transform()
3 participants