Update srm.py#371
Conversation
Added code based on what is in rsrm to allow for transformation of new participant data without changing the shared response s
|
Jenkins, retest this please. |
|
@CameronTEllis, the Jenkins Linux failure is unrelated. I am working on it. We need tests for the new code. |
Added tests for new SRM code (transform_subject addition)
Update test_srm.py
TuKo
left a comment
There was a problem hiding this comment.
Hi Cameron,
The PR looks good.
Please pay attention to the for-loops that need to be removed.
| ---------- | ||
|
|
||
| X : 2D array, shape=[voxels, timepoints] | ||
| The fMRI data of the new subject. |
There was a problem hiding this comment.
Please add that should be under the same stimuli as the original subjects.
| raise ValueError("The number of timepoints(TRs) does not match the" | ||
| "one in the model.") | ||
|
|
||
| for i in range(self.n_iter): |
There was a problem hiding this comment.
There is no need for a for here. You should call the function once.
| raise ValueError("The number of timepoints(TRs) does not match the" | ||
| "one in the model.") | ||
|
|
||
| for i in range(self.n_iter): |
There was a problem hiding this comment.
There is no need for a for-loop here. You should call the function once.
| assert new_s[subject].shape[1] == samples, ( | ||
| "Invalid computation of SRM! (wrong # samples after transform)") | ||
|
|
||
|
|
There was a problem hiding this comment.
Let's add this as part of a new test.
Add a new test function that creates the object and tries to add the new subject
Removed fitting and added to docstring for transform_subject
TuKo
left a comment
There was a problem hiding this comment.
@CameronTEllis thanks for the changes. Please add the missing lines to the test and that is for sure good enough. Thanks!
| X.append(Q.dot(S) + 0.1*np.random.random((voxels, samples))) | ||
|
|
||
| # Check that runs with 2 subject | ||
| s.fit(X) |
There was a problem hiding this comment.
You should test that it does not succeed when the model is not computed before you call fit()
Added test that transforming before fit fails
|
👍 Well done! @mihaic you can go ahead. |
|
@CameronTEllis, you can merge this yourself after you get the tests to pass. |
PEP8
Increasing coverage
Increasing coverage
|
Jenkins, retest this please. |
|
@CameronTEllis, unfortunately, Travis does not respond to that command. Can you please login to Travis and check if you can restart the build? (I am trying to determine if Travis understands GitHub permissions for teams, but I couldn't find an answer yet.) |
Increase coverage
|
@CameronTEllis, to cover lines 403 and 777, you should call Also, it seems to me you have the right and were able to rerun the build on Travis. Can you please confirm? |
Increasing coverage
|
@mihaic Aha thank you for that catch. Hopefully that will fix it. And yes I was able to make an account and rebuild travis thank you |
|
Great, looking good. Unfortunately I still don't have write access to this repo and can't merge the pull request. |
|
My bad. Please try again. And when you write the merge message, have a look at the style of commit messages we use: |
Added code based on what is in rsrm to allow for transformation of new participant data without changing the shared response s