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

FIX: Update spline fitting distance for BOLD bias-field correction #1603

Merged
merged 4 commits into from
May 6, 2019
Merged

FIX: Update spline fitting distance for BOLD bias-field correction #1603

merged 4 commits into from
May 6, 2019

Conversation

markushs
Copy link
Contributor

@markushs markushs commented May 3, 2019

Changes proposed in this pull request

Adding -b [200] as input to ants.N4BiasFieldCorrection in bold_bold_trans_wf/bold_reference_wf/enhance_and_skullstrip_bold_wf/n4_correct.
Previously, default setting (i.e., "single mesh element over the entire domain, -b [1x1x1,3]) was used. This seems to fix issues reported in #1460

Documentation that should be reviewed

Not sure, but don't think docs need to be updated.

Copy link
Member

@effigies effigies left a comment

Choose a reason for hiding this comment

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

LGTM. Have you tried running it? If you're using fmriprep-docker, you can use the -f option to patch your repository into the image for testing.

Copy link
Member

@oesteban oesteban left a comment

Choose a reason for hiding this comment

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

Very much appreciated. @markushs, do you want to add yourself to the .zenodo.json file?

@markushs
Copy link
Contributor Author

markushs commented May 3, 2019

@effigies Thanks! I'll try to run a mix of previously good and bad subs on the patched version tonight (European time). Hopefully the fix solves all issues mentioned in #1460, not only the strange-looking n4-outcomes (I was deliberately vague in my PR...)

@markushs
Copy link
Contributor Author

markushs commented May 3, 2019

@effigies So I tried fmriprep-docker and the -f option to patch my repository into the image:

fmriprep-docker \
-f /Users/markushs/Documents/GitHub/fmriprep/fmriprep \
-i poldracklab/fmriprep:1.3.2 \
/Users/markushs/Downloads/tmp/bids \
/Users/markushs/Downloads/tmp/derivatives \
participant \
-w /Users/markushs/Downloads/tmp/scratch \
--fs-license-file /Users/markushs/Dropbox/fMRI_tools/freesurfer6.0/license.txt \
--output-space T1w template \
--template MNI152NLin2009cAsym \
--nthreads 6 \
--use-plugin /Users/markushs/Downloads/tmp/fmriprep_plugin.yml \
--skip_bids_validation

However I get the following errors when running:

Traceback (most recent call last):
  File "/usr/local/miniconda/lib/python3.7/multiprocessing/process.py", line 297, in _bootstrap
    self.run()
  File "/usr/local/miniconda/lib/python3.7/multiprocessing/process.py", line 99, in run
    self._target(*self._args, **self._kwargs)
  File "/usr/local/miniconda/lib/python3.7/site-packages/fmriprep/cli/run.py", line 590, in build_workflow
    from niworkflows.reports import generate_reports
ModuleNotFoundError: No module named 'niworkflows.reports'
Traceback (most recent call last):
  File "/usr/local/miniconda/bin/fmriprep", line 11, in <module>
    load_entry_point('fmriprep==1.3.2', 'console_scripts', 'fmriprep')()
  File "/usr/local/miniconda/lib/python3.7/site-packages/fmriprep/cli/run.py", line 395, in main
    bids_dir = Path(retval.get('bids_dir'))
  File "/usr/local/miniconda/lib/python3.7/pathlib.py", line 994, in __new__
    self = cls._from_parts(args, init=False)
  File "/usr/local/miniconda/lib/python3.7/pathlib.py", line 649, in _from_parts
    drv, root, parts = self._parse_args(args)
  File "/usr/local/miniconda/lib/python3.7/pathlib.py", line 633, in _parse_args
    a = os.fspath(a)
TypeError: expected str, bytes or os.PathLike object, not NoneType

I assume this is due to my repository being a clone of the current "unstable" master which contains changes not reflected in fmriprep:1.3.2. I tried patching in the clone of the current niworkflows-master (using the -n option), but got errors here as well.

Is there a quickfix for this? In the meantime I'll try running fmriprep-docker with -i poldracklab/fmriprep:unstable.

@effigies
Copy link
Member

effigies commented May 3, 2019

Ah. Looks like you're going to need to patch in the latest niworkflows, as well. One of the hazards of splitting development across projects. You can use the -n flag for that.

@effigies
Copy link
Member

effigies commented May 3, 2019

Is there a quickfix for this? In the meantime I'll try running fmriprep-docker with -i poldracklab/fmriprep:unstable.

Oh, this should work, too.

@markushs
Copy link
Contributor Author

markushs commented May 3, 2019

@effigies Hi again, and sorry for pestering you, but new errors show up when running fmriprep:unstable, independent of whether I patch in the latest niworkflows or not.

190503-17:42:17,813 nipype.workflow IMPORTANT:
	 
    Running fMRIPREP version 0+unknown:
      * BIDS dataset path: /data.
      * Participant list: ['1000626', '1100143', '1100322', '1510005', '1510131'].
      * Run identifier: 20190503-174217_2435fd93-e87c-4f75-8db4-e8c07005fac9.
    
Process Process-2:
Traceback (most recent call last):
  File "/usr/local/miniconda/lib/python3.7/multiprocessing/process.py", line 297, in _bootstrap
    self.run()
  File "/usr/local/miniconda/lib/python3.7/multiprocessing/process.py", line 99, in run
    self._target(*self._args, **self._kwargs)
  File "/usr/local/miniconda/lib/python3.7/site-packages/fmriprep/cli/run.py", line 803, in build_workflow
    err_on_aroma_warn=opts.error_on_aroma_warnings,
  File "/usr/local/miniconda/lib/python3.7/site-packages/fmriprep/workflows/base.py", line 218, in init_fmriprep_wf
    err_on_aroma_warn=err_on_aroma_warn,
  File "/usr/local/miniconda/lib/python3.7/site-packages/fmriprep/workflows/base.py", line 462, in init_single_subject_wf
    skull_strip_template=skull_strip_template,
TypeError: init_anat_preproc_wf() got an unexpected keyword argument 'output_spaces'

I guess the current "unstable" master truly is unstable (at least until this one #1605 is merged)?

@oesteban
Copy link
Member

oesteban commented May 3, 2019

I would recommend waiting on #1596 to be merged first.

Have you thought of adding yourself to the zenodo file?

@effigies
Copy link
Member

effigies commented May 3, 2019

That's strange... The current master should have been tested and not produce this issue. Possibly you need to docker pull poldracklab/fmriprep:unstable again?

@effigies
Copy link
Member

effigies commented May 3, 2019

The other thing you can do is just docker build -t poldracklab/fmriprep:testing . in your repository. That should produce a usable image.

@markushs
Copy link
Contributor Author

markushs commented May 3, 2019

@oesteban I would be happy and honoured to be a part of the zenodo file! I'll add my name as soon as this PR has proven that it actually fixes something (fingers crossed).

And @effigies , I dockerpulled unstable 30min ago, but will try your second suggestion.

@oesteban
Copy link
Member

oesteban commented May 4, 2019

Hi @markushs, I think you can rebase to current master. The dependency issues should be resolved now.

Some guidelines for the zenodo file are given here: #1564 (comment)

@effigies effigies added this to the 1.4.0 milestone May 4, 2019
@markushs
Copy link
Contributor Author

markushs commented May 6, 2019

So, I can finally confirm that all of the issues we reported in #1460 are gone after rerunning with the fix. And no new issues seem to appear (i.e., runs without issues before the fix still look fine). I ran the tests on a total of 42 runs from 6 participants.

@markushs
Copy link
Contributor Author

markushs commented May 6, 2019

Ah, I should have just updated the zenodo in this branch. Ended up as a separate PR, sorry about that.

@effigies effigies changed the title FIX: change in default input parameters to N4BiasFieldCorrection (n4_correct) FIX: Update spline fitting distance for BOLD bias-field correction May 6, 2019
@effigies effigies merged commit 8f231c9 into nipreps:master May 6, 2019
@effigies effigies mentioned this pull request May 6, 2019
@markushs markushs deleted the n4correct_patch branch May 6, 2019 13:46
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