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

Update srm.py #371

Merged
merged 13 commits into from May 15, 2018
Merged

Update srm.py #371

merged 13 commits into from May 15, 2018

Conversation

@CameronTEllis
Copy link
Contributor

@CameronTEllis CameronTEllis commented May 4, 2018

Added code based on what is in rsrm to allow for transformation of new participant data without changing the shared response s

Added code based on what is in rsrm to allow for transformation of new participant data without changing the shared response s
@lcnature lcnature requested a review from TuKo May 4, 2018
@mihaic
Copy link
Contributor

@mihaic mihaic commented May 4, 2018

Jenkins, retest this please.

@mihaic
Copy link
Contributor

@mihaic mihaic commented May 4, 2018

@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
Copy link
Contributor

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

This comment has been minimized.

@TuKo

TuKo May 4, 2018
Contributor

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

This comment has been minimized.

@TuKo

TuKo May 4, 2018
Contributor

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

This comment has been minimized.

@TuKo

TuKo May 4, 2018
Contributor

There is no need for a for-loop here. You should call the function once.

@@ -98,7 +98,14 @@ def test_can_instantiate():
"Invalid computation of SRM! (wrong # features after transform)")
assert new_s[subject].shape[1] == samples, (
"Invalid computation of SRM! (wrong # samples after transform)")


This comment has been minimized.

@TuKo

TuKo May 4, 2018
Contributor

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
Copy link
Contributor

@TuKo TuKo left a 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)

This comment has been minimized.

@TuKo

TuKo May 7, 2018
Contributor

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
@TuKo
TuKo approved these changes May 11, 2018
@TuKo
Copy link
Contributor

@TuKo TuKo commented May 11, 2018

👍 Well done! @mihaic you can go ahead.

@mihaic
Copy link
Contributor

@mihaic mihaic commented May 11, 2018

@CameronTEllis, you can merge this yourself after you get the tests to pass.

PEP8
PEP8
PEP8
Increasing coverage
Increasing coverage
@CameronTEllis
Copy link
Contributor Author

@CameronTEllis CameronTEllis commented May 15, 2018

Jenkins, retest this please.

@mihaic
Copy link
Contributor

@mihaic mihaic commented May 15, 2018

@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
@mihaic
Copy link
Contributor

@mihaic mihaic commented May 15, 2018

@CameronTEllis, to cover lines 403 and 777, you should call transform_subject before fitting (you are calling transform).

Also, it seems to me you have the right and were able to rerun the build on Travis. Can you please confirm?

Increasing coverage
@CameronTEllis
Copy link
Contributor Author

@CameronTEllis CameronTEllis commented May 15, 2018

@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

@CameronTEllis
Copy link
Contributor Author

@CameronTEllis CameronTEllis commented May 15, 2018

Great, looking good. Unfortunately I still don't have write access to this repo and can't merge the pull request.

@mihaic
Copy link
Contributor

@mihaic mihaic commented May 15, 2018

My bad. Please try again. And when you write the merge message, have a look at the style of commit messages we use:
https://github.com/brainiak/brainiak/commits/master

@CameronTEllis CameronTEllis merged commit b81adac into brainiak:master May 15, 2018
5 checks passed
5 checks passed
codecov/patch 100% of diff hit (target 88.7%)
Details
codecov/project 88.88% (+0.18%) compared to a16df00
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
linux Build finished.
Details
macos Build finished.
Details
@CameronTEllis CameronTEllis deleted the CameronTEllis:patch-2 branch May 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants