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

Changes to fix issue #361: matrix sizing in tracking.utils.connectivity_matrix #362

Merged
merged 3 commits into from May 6, 2014

Conversation

Projects
None yet
3 participants
@AndrewLawrence
Contributor

AndrewLawrence commented May 6, 2014

Fixes for edge case where tract endpoints do not intersect with last label (or labels) from a supplied labelset.

@MrBago

This comment has been minimized.

Contributor

MrBago commented May 6, 2014

This looks great Andrew. Could you do one more thing, could you write a test for this in dipy.tracking.tests.test_utils? You can call it test_connectivity_matrix_shape or something similar.

If I had written the test in the first place, we never would have had this bug, but better late then never :). The test should be a very simple example that reproduces the bad behavior and fails if it's run without the fix.

If you commit the test and push the branch again github will update the pull request.

@arokem

This comment has been minimized.

Member

arokem commented May 6, 2014

Hey @AndrewLawrence. Thanks so much for your contribution! We are always delighted to have new people join and contribute! One comment: for a bug-fix, we usually ask that the fix be accompanied by a test that fails without the fix. This prevents the bug from creeping back, and improves the utility of the test-suite. If you need help figuring out how to write a test for this, let us know and we will help along. Thanks again!

@arokem

This comment has been minimized.

Member

arokem commented May 6, 2014

As you can see, we are quite passionate about this testing business :-)

@AndrewLawrence

This comment has been minimized.

Contributor

AndrewLawrence commented May 6, 2014

:) Thanks for the welcome all, I'll write a test and see how it goes.

@AndrewLawrence

This comment has been minimized.

Contributor

AndrewLawrence commented May 6, 2014

Hi there, test works with fix and doesn't work without it.
Please do check I've got the right idea, this is my first test of this type.
Thanks,

Andrew

def test_connectivity_matrix_shape():
# Labels: z-planes have labels 0,1,2
labels = np.zeros((3,3,3))

This comment has been minimized.

@MrBago

MrBago May 6, 2014

Contributor

You can pass the dtype to zeros right here so you don't have to cast to int later, ie labels = np.zeros((3, 3, 3), dtype=int).

@MrBago

This comment has been minimized.

Contributor

MrBago commented May 6, 2014

This looks really good, nice test. In dipy we use the pep8 style guidelines, which might be good to read if you aren't familiar with them. The only thing I see that's an issue is spaces after commas. I know this is a bit nitpicky, but since travis seems to have chocked and we need another commit to get travis to re-run the tests do you mind fixing this and the dtype issue.

@AndrewLawrence

This comment has been minimized.

Contributor

AndrewLawrence commented May 6, 2014

Thanks, I'll try to keep in pep8 for the future, it is more readable that way.
I've fixed the dtype and it now seems to be passing the Travis build.

@AndrewLawrence

This comment has been minimized.

Contributor

AndrewLawrence commented May 6, 2014

hmm, but now it's in progress again.

@arokem

This comment has been minimized.

Member

arokem commented May 6, 2014

That's because you closed and reopened it. No worries.

On Tue, May 6, 2014 at 1:16 PM, AndrewLawrence notifications@github.comwrote:

hmm, but now it's in progress again.


Reply to this email directly or view it on GitHubhttps://github.com//pull/362#issuecomment-42352979
.

MrBago added a commit that referenced this pull request May 6, 2014

Merge pull request #362 from AndrewLawrence/master
Fix bug with connectivity matrix shape, issue #361.

@MrBago MrBago merged commit 661de17 into nipy:master May 6, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@MrBago

This comment has been minimized.

Contributor

MrBago commented May 6, 2014

I love easy pull requests :). Welcome to the team @AndrewLawrence.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment