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

Integrating amp2response in dwi2response scripts #862

Merged
merged 22 commits into from Dec 22, 2016

Conversation

Projects
None yet
1 participant
@thijsdhollander
Copy link
Contributor

commented Dec 13, 2016

Additionally, default lmax=10 for anisotropic responses. Follows some discussion in #786.

@thijsdhollander thijsdhollander self-assigned this Dec 19, 2016

@thijsdhollander thijsdhollander merged commit 0bfc397 into tag_0.3.16 Dec 22, 2016

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@thijsdhollander

This comment has been minimized.

Copy link
Contributor Author

commented Dec 22, 2016

So for reference: what I've done here is as a primary thing up the lmax to 10 and integrate amp2response in the dwi2response scripts. However, as I was doing that, I came across a range of bugs, most of which I reason originated because we once considered single-shell as the default data that typically came in, as well as due to some stuff making other assumptions about the users' input. For instance, dwi2response tax was actually grossly wrong when multi-shell data came in: the only way the data was processed when it came in, was (twice: once in the general script, and once in the tax code itself) a simple dwiextract. All good for actual single-shell data, but for multi-shell data everything (but the bzeros) came in. For most single-shell response algos, that was not a major issue (apart from wasting disk space and time), because the amp2sh took care of it; but e.g. in the tax algorithm, the amplitudes of the dwi.mif were also used, assuming they were a single shell (but they aren't in case of multi-shell data).

Anyways, that led me to reconsider some small aspects of the main dwi2response script, so single-shell algos would only get the real single-shell part. However, that led me further to look at dwiextract, to see if useful functionality could be added to that one to make this all a bit easier (for scripts in general). I thought that would be a simple one as well, as there already appeared to be such functionality in the select_shells method in shells.cpp. So I added it, tested it, and then stumbled across a range of unexpected behaviours / bugs in select_shells. To just name a couple (there were many more): selecting shells like 3000,0 under a single shell regime would fail as if it where multiple non-zero b-values. Assumption made here was that the user would select 0 as the first one. Things like 5,3000 would fail here too, because even though 5 would translate into a bzero, it wasn't detected at that check. Things like 0,3000,3005 would go wrong often as well, again because checks where on what the user provided, but didn't take into account into what selection such things translate; so this may result in commands not actually getting the kind of data they desire, and the users potentially not being aware of this happening. And if there was no "-shells" option, but the single-shell constraint was demanded, but not the bzero, then the bzero got removed if the data was already single-shell out of the box, but not if it was multi-shell out of the box (then suddenly the bzero was in). And so on, and so on...

Long story short: I could actually see how some of these bugs had been introduced over time, and it was quite in line with an old single-shell logic that was migrated to a multi-shell one, but via smaller disconnected hacks over time. The select_shells code was also not the easiest one to navigate through, due to lots of nested logic going on in there. So I wrapped my mind around all of that, and rewrote the bits that had to do with these various optional constraints that you can ask from select_shells. The bugs are ironed out (and lots of extra checks have been added), and the behaviour and functionality offered to developers should be more clear now. Whatever was possible before, is still possible (and more), and apart from bugs, the behaviour of commands that uses this functionality is still the same (I took care of each separate occurrence to consider this well, and tested it extensively). The relevant documentation that explains the behaviour (so future developers don't have to navigate the code of this nested logic to find out) was added here. This should give them all the tricks I can imagine would be requirements that they may have for incoming dMRI data. Also no worries about the shell assignment logic: this part of it is unaffected.

Since having this functionality may also be useful when writing certain scripts, I basically offered it externally via the dwiextract interface as well; again no worries: everything is backwards compatible, no existing behaviours have been changed, and for a simple dwiextract or dwiextract -bzero, it still doesn't initialise the Shells class, as before. The new features are only relevant if people start to play around with shells, or are having shell requirements, anyway; so in a context where we're definitely dealing with shelled data.

Finally, there's also an added -isotropic option for amp2response. It's a shortcut for -lmax 0,0,0,... essentially, but it's more easy to use, and independent of how many shells there are. It's also just quite nice in context of scripts that e.g. get WM, GM and CSF responses, where the latter is/are isotropic; makes those scripts very readable as well.

All of this combined also resulted in a much smaller disk footprint of some dwi2response algos: no more SH images that have to be stored in between, single-shell algos only copying the part of the dMRI dataset they really need, etc...

@thijsdhollander

This comment has been minimized.

Copy link
Contributor Author

commented Dec 22, 2016

Maybe final finding I should mention: the output of all dwi2response algos will now of course not be exactly the same any more compared to before (and it's lmax=10 by default), but where this could potentially have had a greater impact, was with iterative algorithms like the tournier one, as the output is used to initialise each next iteration. However, I've tested this in a range of scenarios, and the output (and the final voxel selection) is very close to the same in all cases where the data quality is ok/fine. In some cases, the voxel selection even ends up being exactly the same as before, which I reckon is a good thing. The tax algorithm is now amazingly slow though, especially in the first or earlier iterations in general (but I haven't compared to before, it may already have been quite slow...?). The huge number of voxels it feeds to amp2response early on (and even still quite a bit later on) is the main cause of this. Oh yeah, and final thing now that I'm mentioning the tax algorithm: I also ported it to using the new fixel format while I was at it. It seems @draffelt may have overlooked this one. 😉

@thijsdhollander thijsdhollander deleted the integrate_amp2response branch Dec 22, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.