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

Omnibus multi-echo pull request #1296

Merged
merged 51 commits into from
Nov 29, 2018
Merged

Omnibus multi-echo pull request #1296

merged 51 commits into from
Nov 29, 2018

Conversation

effigies
Copy link
Member

@effigies effigies commented Sep 25, 2018

This PR tracks a series of updates to multi-echo preprocessing, an effort being led by @emdupre. The work has been broken into multiple PRs to ensure that each is independently reviewable.

New multi-echo related PRs should target the multiecho branch of the main repository, rather than master.

Any updates to master should be merged into multiecho as soon as possible to avoid getting out of sync.

Refactors that substantially affect the BOLD workflows should consider targeting the multiecho branch unless updates to master are needed before multiecho is likely to be merged.

Changes proposed in this pull request

Closes #1034, #1135

@emdupre
Copy link
Collaborator

emdupre commented Oct 27, 2018

I'm a little confused how merging in a pull request to merge in master resulted in new merge conflicts ! Do you want me to try and fix the conflict here, @effigies ?

@effigies
Copy link
Member Author

It was fine, but there've since been changes to master. I think if we wait for #1354, the conflicts will go away again.

@emdupre
Copy link
Collaborator

emdupre commented Nov 6, 2018

@effigies Looking back at previous conversations from #1324 (comment), how would I go about modifying bbregister parameters ? Did you want me to loop in someone, or should I test them myself ? Or should we instead follow previous suggestions from @oesteban (see: #1034) and modify the T2* map itself ?

@effigies
Copy link
Member Author

effigies commented Nov 6, 2018

Ah, reading #1034, it might be good to get ahold of a "failing" T2* map to see if we can assess the issues. If we're confident that it's not negative numbers and outliers, then we can go ahead and ping the FreeSurfer list to see if there are any tweakable parameters to try for bbregister. Are you comfortable experimenting with the directions @oesteban suggested? Also happy to provide more specific suggestions.

@emdupre
Copy link
Collaborator

emdupre commented Nov 7, 2018

Yes, happy to. It looks like merging in master broke something here, though, so maybe I'll try to fix that first !

@effigies
Copy link
Member Author

effigies commented Nov 7, 2018

Ah, thanks for the heads up. I also apparently failed to read you saying you'd fix it, so I think I fixed it.

@emdupre
Copy link
Collaborator

emdupre commented Nov 12, 2018

I had pushed this off until I had a little more time to sit down with it, but the solution was actually easier than I originally anticipated.

Essentially, when calculating the T2* map we have the problem of what to do in voxels where only one echo has reliable signal. For the standard T2* map we simply exclude those voxels, as their values can be unpredictable. For the adaptive T2* map-- which I had originally set as the coregistration target in the case of -t2s-coreg we retain them and treat them at face value.

When I switched to the more stable, "standard" T2* map, the 10% cases vanished. The poorly behaving example I had shown previously (among others) now looks as expected, see:

screenshot_2018-11-11 screenshot

I've opened #1383 to address this change and to make sure that the reports and all other relevant documentation is up-to-date. But I'm feeling optimistic that this is quite close !

@effigies
Copy link
Member Author

Sounds good to me.

@effigies effigies added this to the 1.3.0 milestone Nov 29, 2018
@effigies effigies changed the title [WIP] Omnibus multi-echo pull request Omnibus multi-echo pull request Nov 29, 2018
@effigies effigies merged commit 1bc3c35 into master Nov 29, 2018
@oesteban oesteban deleted the multiecho branch March 6, 2019 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants