Compute cluster label maps using Python (rather than depending on FSL)

incf-nidash / nidmresults-fsl

A python library to export FSL's feat results to NIDM-Results

http://nidm.nidash.org/specs/nidm-results.html

MIT License

3 stars 11 forks source link

Compute cluster label maps using Python (rather than depending on FSL) #134

Closed cmaumet closed 6 years ago

cmaumet commented 6 years ago

This PR updates the codebase to rely on scipy rather than FSL to compute the cluster labels map:

This removes one call to the cluster command as suggested in #114 (and also the hard-coded re-thresholding at 0.01)
It adds scipy as a requirement.

@TomMaullin: can you review this PR?

pep8speaks commented 6 years ago

Hello @cmaumet! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. :beers:

Comment last updated on July 12, 2018 at 16:02 Hours UTC

cmaumet commented 6 years ago

Tests are currently failing because the shasum of the cluster label maps has changed. This is probably related to issue identified by @TomMaullin (more to come on this) that clusters will not necessarily have the same cluster id than the ones provided by FSL. But other changes in the header might also explain the difference in shasum. Maybe we should remove this check from the tests.

nicholst commented 6 years ago

Can we instead do a comparison of the cluster patterns? In fslmaths I'd do it like

fslmaths clus_index1 -bin tmp1 fslmaths clus_index2 -bin tmp2 fslmaths tmp1 -sub tmp2 -abs tmp3 fsltats -R tmp3 # check this is 0 0

Of course I'm sure there are more elegant ways to do it in python.

cmaumet commented 6 years ago

@nicholst: Thanks for this! Yes, we could change the tests to compare the image as you suggest and leave the shasum aside.

cmaumet commented 6 years ago

This new set of updates:

Relabel the clusters to match FSL's labels --> fix #136
Take into account the connectivity criterion set in FSL (6- 18- or 26-connected) when computing the cluster label map

cmaumet commented 6 years ago

Tests pass again! @TomMaullin: I've added a comment for the 10000, let me know if there is anything else.

cmaumet commented 6 years ago

Note: I've interrupted the tests for the previous commits which is why they appear as failed. The last run should be okay (hopefully!).

TomMaullin commented 6 years ago

I think that's now everything I can think of! So long as the tests still pass and all of the suggestions you think are good have been addressed, I am happy to merge when you are!

cmaumet commented 6 years ago

Great! Thanks for the review! Let's see if the test pass and if that's the case one of us can merge :)

cmaumet commented 6 years ago

As agreed with @TomMaullin above, let's merge!