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.

Sign up for GitHub

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

Jump to bottom

Running NewSegment properly #258

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Running NewSegment properly #258

wants to merge 2 commits into from

Conversation

mrahim
Copy link
Collaborator

@mrahim mrahim commented Jan 16, 2017

Trying to address #77

  • NewSegment is now run in parallel with or without DARTEL with do_subject_newsegment
  • DARTEL is run separately from Newsegment in do_subjects_dartel
  • Normalize12 is used with NewSegment in do_subject_normalize

@mrahim mrahim changed the title Running Newsegment properly Running NewSegment properly Jan 16, 2017
bthirion
bthirion reviewed Jan 17, 2017
View reviewed changes
Copy link
Member

@bthirion bthirion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, but having a test would be much better imho

pypreprocess/nipype_preproc_spm_utils.py
@@ -74,11 +74,11 @@ def _configure_backends(spm_dir=None, matlab_exec=None, spm_mcr=None,

# prepare template TPMs
tissue1 = ((os.path.join(SPM_DIR, tissue_path, 'TPM.nii'), 1),
2, (True, True), (False, False))
2, (True, True), (True, True))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A short comment on these parameters would be welcome

pypreprocess/nipype_preproc_spm_utils.py

normalize: bool, optional (default False)
flag indicating whether warped brain compartments (gm, wm, csf) are to
be generated (necessary if the caller wishes the brain later)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a flag related to spm.Segment. It doesn't hold for spm.NewSegment: to be removed.

pypreprocess/nipype_preproc_spm_utils.py Outdated
# final hard link
for subject_data in subjects:
subject_data.hardlink_output_files(final=True)

finalize_report()
return subjects
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we have a test that takes an anat image as input and checks that tpms are generated and / or normalized ?

@dohmatob
Copy link
Contributor

This doesn't look good.

logo

@mrahim
Copy link
Collaborator Author

mrahim commented Jan 17, 2017

Because of SPM8, I've tested under SPM12 and it's working.

@bthirion
Copy link
Member

LGTM now.

@dohmatob
Copy link
Contributor

As long as the fsl_feeds example on circle-ci looks ugly (see thumbnail above), this looks like a regression to me. Could you please propagate your changes so that circle-ci looks good ?

@dohmatob
Copy link
Contributor

dohmatob commented Apr 21, 2017 •
edited
Loading

Looks like this PR is going to rot here really fast :/. Any updates on the raise issues ?

@bthirion
Copy link
Member

No, please merge it. We badly need it.

@mrahim
Copy link
Collaborator Author

mrahim commented Apr 22, 2017 •
edited
Loading

I think we need to drop spm8 support.
With spm12, everything is okay.

https://268-28154958-gh.circle-artifacts.com/0/home/ubuntu/nilearn_data/fsl_feeds/pypreprocess_output/sub001/reports/report_preproc.html

subject_tpms_contours_on_subject_ warped anatomical_image

@mrahim mrahim mentioned this pull request Apr 22, 2017
Merged
@bthirion
Copy link
Member

LGTM.

@bthirion bthirion mentioned this pull request Feb 14, 2018
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bthirion bthirion bthirion left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mrahim @dohmatob @bthirion