Skip to content

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

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

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

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

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

return U.dot(V), mu

def transform_subject(self, X):
"""Transform a new subject using the existing model.
Copy link

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