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

use freesurfer segmentation for bbr #285

Merged
merged 2 commits into from Jan 30, 2019

Conversation

Projects
None yet
6 participants
@alebel14
Copy link
Contributor

alebel14 commented Aug 15, 2018

This code imports the whitematter segmentation from freesurfer instead of using the unreliable segmentation from fast. This should solve issue #284 .

@alexhuth

This comment has been minimized.

Copy link
Contributor

alexhuth commented Aug 15, 2018

Just adding -- @alebel14 has tested this and it seems to work very nicely! No more screwing around with the incredibly inaptly named fast, just use the high-quality white matter segmentation from freesurfer.

@anwarnunez

This comment has been minimized.

Copy link
Contributor

anwarnunez commented Aug 16, 2018

so cool! thanks for the fix @alebel14!

talked to @jamesgao
if we gonna do this, we should use freesurfer BBR everywhere.

please test backwards compatibility though. that is, previously imported surfaces should still be usable (e.g.automatic alignment with BBR should not break with new code). this will also have downstream effects on preproc pipelines, which should also be tested.

@marklescroart

This comment has been minimized.

Copy link
Contributor

marklescroart commented Sep 17, 2018

@alexhuth, do you think this has been tested thoroughly enough to merge? Do @anwarnunez or @jamesgao have strong feelings about what, specifically, should also be done on this branch? (What do you mean by "use freesurfer BBR everywhere" - which other functions need modification? Do you mean we should eliminate all calls to FSL?)

@anwarnunez

This comment has been minimized.

Copy link
Contributor

anwarnunez commented Sep 17, 2018

I was referring to the use of BBR in cortex.align. It should also be changed to use freesurfer.

@anwarnunez

This comment has been minimized.

Copy link
Contributor

anwarnunez commented Sep 17, 2018

Also, there was a meat-space discussion with @jamesgao @fatmai about moving forward with this. There is the issue of backwards compatibility. I can't remember the resolution. Hoping james or fatma can jump int.

@alebel14 : Have y'all had a chance to test backwards compability? Thanks!

Anwar

@alebel14

This comment has been minimized.

Copy link
Contributor Author

alebel14 commented Sep 17, 2018

Hi, @anwarnunez , I've tested it for backwards compatibility and found no issue there.
I've been using this update since i posted it and have not come across any issues myself at least.

@fatmai

This comment has been minimized.

Copy link
Contributor

fatmai commented Sep 18, 2018

To keep backwards compatibility we decided to keep the freesurfer and FSL created wm files wm_freesurfer.nii.gz and wm_fsl.nii.gz in the anatomicals folder of the subject. By default load the freesurfer version when it is there and the fsl version if not. (@jamesgao please correct if there is something missing.)

Also as @anwarnunez mentioned we have to update cortex.align to use the freesurfer BBR in that case.

@alexhuth

This comment has been minimized.

Copy link
Contributor

alexhuth commented Sep 18, 2018

@fatmai to me, that actually seems like it would work against backwards compatibility, since the current subjects only have a wm.nii.gz We'd have to write a load function that checks first for wm_freesurfer.nii.gz, then falls back to wm_fsl.nii.gz and finally falls back to wm.nii.gz.

As it is currently (in the patch we're discussing), the loading function always only looks for wm.nii.gz, which might be generated by FSL or imported from freesurfer. If that file is missing (meaning that it wasn't created when the subject was imported from freesurfer), it tries to generate it using FSL. This is perfectly backwards compatible, and requires no special-casing of different possible versions of the file.

The only downside of the current patch is that it doesn't make explicit where wm.nii.gz comes from--it might be from FSL or it might be from freesurfer, and you don't have a clear way to tell. But that's something that I'd prefer we fix through some other mechanism (e.g. writing a header indicating provenance into wm.nii.gz when it's imported or constructed by FSL).

@alexhuth

This comment has been minimized.

Copy link
Contributor

alexhuth commented Sep 18, 2018

jk I was misremembering, and just re-checked what actually happens:

  1. The freesurfer wm segmentation is imported as raw_wm.nii.gz at import time (i.e. when the subject is created in the pycortex db)
  2. When the wm volume is requested, this first tries to convert the freesurfer raw_wm (through a couple steps)
  3. If that fails (e.g. if there is no raw_wm because it's an old subject) it uses FSL to create wm.nii.gz
  4. In either case (2 or 3) the resulting segmentation is saved as wm and used thenceforth.

This all seems sensible to me. There's provenance: if there is a raw_wm.nii.gz file, then you know that the freesurfer segmentation was used. Otherwise you know the FSL segmentation was used. It's not the most explicit, but hey..

@eickenberg eickenberg force-pushed the alebel14:add_freesurfer_wm_segmentation branch from 0c3d688 to aa787e9 Dec 8, 2018

@eickenberg

This comment has been minimized.

Copy link
Contributor

eickenberg commented Dec 8, 2018

fyi I just rebased this

@anwarnunez

This comment has been minimized.

Copy link
Contributor

anwarnunez commented Jan 30, 2019

Seems like this is all resolved. It should be merged.

Unless anyone beats me to it, I'll merge by end of day.

@anwarnunez anwarnunez merged commit 2191dfd into gallantlab:master Jan 30, 2019

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