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: --cifti-output missing required space #212

Merged
merged 15 commits into from
May 11, 2022
Merged

Conversation

mgxd
Copy link
Collaborator

@mgxd mgxd commented Apr 22, 2022

Addressing #211 - first step will be to add --cifti-output to circle testing

@mgxd
Copy link
Collaborator Author

mgxd commented Apr 25, 2022

@madisoth could you verify this patch fixes your issue? I pushed a docker image with this patch to nipreps/nibabies:fix-cifti-output.

@madisoth
Copy link
Contributor

Hi @mgxd, I got a test subject processed and the cifti output looks good.

But I did hit a separate error; think it's just this fMRIPlot call needs to be updated to work with niworkflows after the bump to 1.5.3

File: /out/sub-<SUBJECT>/log/20220429-144654_9d9649e2-9e55-4899-9115-d4936fd67b2d/crash-20220429-203608-tmadison-conf_plot-8d2bbe66-b1cb-4484-9fc2-3c581aedf9e0.txt
Working Directory: /work/nibabies_22_0_wf/single_subject_<SUBJECT>_wf/func_preproc_ses_<SESSION>_task_auditory_run_01_wf/carpetplot_wf/conf_plot
Inputs: 
confounds_file:
confounds_list: [('global_signal', None, 'GS'), ('csf', None, 'GSCSF'), ('white_matter', None, 'GSWM'), ('std_dvars', None, 'DVARS'), ('framewise_displacement', 'mm', 'FD')]
in_func:
in_mask:
in_segm:
str_or_tuple:
tr: 1.2
Traceback (most recent call last):
  File "/opt/conda/lib/python3.8/site-packages/nipype/pipeline/plugins/multiproc.py", line 67, in run_node
    result["result"] = node.run(updatehash=updatehash)
  File "/opt/conda/lib/python3.8/site-packages/nipype/pipeline/engine/nodes.py", line 516, in run
    result = self._run_interface(execute=True)
  File "/opt/conda/lib/python3.8/site-packages/nipype/pipeline/engine/nodes.py", line 635, in _run_interface
    return self._run_command(execute)
  File "/opt/conda/lib/python3.8/site-packages/nipype/pipeline/engine/nodes.py", line 741, in _run_command
    result = self._interface.run(cwd=outdir)
  File "/opt/conda/lib/python3.8/site-packages/nipype/interfaces/base/core.py", line 428, in run
    runtime = self._run_interface(runtime)
  File "/opt/conda/lib/python3.8/site-packages/nibabies/interfaces/confounds.py", line 413, in _run_interface
    fig = fMRIPlot(
TypeError: __init__() got an unexpected keyword argument 'mask_file'

@mgxd
Copy link
Collaborator Author

mgxd commented May 2, 2022

oof, yes niworkflow 1.5.x series included some API breaking changes. I'm going to try to get 22.0.x in a stable state, and then start on 22.1.x to alleviate this.

I've backported an older version of fMRIPlot directly into the code - I've pushed it to this branch https://github.com/nipreps/nibabies/tree/docker/fix-cifti-output

You can either:

  1. Wait until the Docker patch builds and build a new image
  2. Clone that repo and directly patch it in to the image you used above

Sorry for this hassle, and thanks for helping debug.

@pep8speaks
Copy link

pep8speaks commented May 2, 2022

Hello @mgxd! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2022-05-11 17:30:34 UTC

@mgxd mgxd force-pushed the fix/cifti-output branch 2 times, most recently from 6f32234 to c67359c Compare May 2, 2022 16:11
@mgxd mgxd changed the title CI: Increase machine resources, add --cifti-output to testing FIX: --cifti-output missing required space May 2, 2022
@mgxd mgxd marked this pull request as ready for review May 4, 2022 20:26
@mgxd
Copy link
Collaborator Author

mgxd commented May 4, 2022

following up - did the latest patch end up fixing the error you ran into @madisoth?

nibabies/utils/viz.py Outdated Show resolved Hide resolved
@madisoth
Copy link
Contributor

madisoth commented May 5, 2022

Getting this error now-- just need to check whether sort_rows is str I think? (see above)

220504-22:40:25,633 nipype.workflow WARNING:
	 Storing result file without outputs
220504-22:40:25,642 nipype.workflow WARNING:
	 [Node] Error on "nibabies_22_0_wf.single_subject_<SUBJECT>_wf.func_preproc_ses_<SESSION>_task_auditory_run_01_wf.carpetplot_wf.conf_plot" (/work/nibabies_22_0_wf/single_subject_<SUBJECT>_wf/func_preproc_ses_<SESSION>_task_auditory_run_01_wf/carpetplot_wf/conf_plot)
220504-22:40:25,976 nipype.workflow ERROR:
	 Node conf_plot failed to run on host acn76.
220504-22:40:25,979 nipype.workflow ERROR:
	 Saving crash info to /out/sub-<SUBJECT>/log/20220504-180819_e143d479-2102-4a3d-8f2c-a591b11b5baa/crash-20220504-224025-tmadison-conf_plot-475d8554-7def-49df-96b8-7e71310a2e51.txt
Traceback (most recent call last):
  File "/opt/conda/lib/python3.8/site-packages/nipype/pipeline/plugins/multiproc.py", line 67, in run_node
    result["result"] = node.run(updatehash=updatehash)
  File "/opt/conda/lib/python3.8/site-packages/nipype/pipeline/engine/nodes.py", line 516, in run
    result = self._run_interface(execute=True)
  File "/opt/conda/lib/python3.8/site-packages/nipype/pipeline/engine/nodes.py", line 635, in _run_interface
    return self._run_command(execute)
  File "/opt/conda/lib/python3.8/site-packages/nipype/pipeline/engine/nodes.py", line 741, in _run_command
    result = self._interface.run(cwd=outdir)
  File "/opt/conda/lib/python3.8/site-packages/nipype/interfaces/base/core.py", line 428, in run
    runtime = self._run_interface(runtime)
  File "/opt/conda/lib/python3.8/site-packages/nibabies/interfaces/confounds.py", line 415, in _run_interface
    fig = fMRIPlot(
  File "/opt/conda/lib/python3.8/site-packages/nibabies/utils/viz.py", line 106, in plot
    plot_carpet(
  File "/opt/conda/lib/python3.8/site-packages/nibabies/utils/viz.py", line 249, in plot_carpet
    return _carpet(
  File "/opt/conda/lib/python3.8/site-packages/nibabies/utils/viz.py", line 305, in _carpet
    if sort_rows.lower() == "linkage":
AttributeError: 'bool' object has no attribute 'lower'

@madisoth
Copy link
Contributor

madisoth commented May 5, 2022

@mgxd the plots issue looks good now, but now I see the medial walls aren't masked out in the 91k dtseries outputs. If I run wb_command -file-information on any of the "91k" func outputs I get this:


CIFTI Dim[0]:                   335
CIFTI Dim[1]:                   96854
ALONG_ROW map type:             SERIES
    Start:                      0.000
    Step:                       1.200
    Units:                      Seconds
ALONG_COLUMN map type:          BRAIN_MODELS
    Has Volume Data:            true
    Volume Dims:                91,109,91
    Volume Space:               2,0,0,-90;0,2,0,-126;0,0,2,-72
    CortexLeft:                 32492 out of 32492 vertices
    CortexRight:                32492 out of 32492 vertices

but I would expect this:

CIFTI Dim[0]:                   335
CIFTI Dim[1]:                   91282
ALONG_ROW map type:             SERIES
    Start:                      0.000
    Step:                       1.200
    Units:                      Seconds
ALONG_COLUMN map type:          BRAIN_MODELS
    Has Volume Data:            true
    Volume Dims:                91,109,91
    Volume Space:               -2,0,0,90;0,2,0,-126;0,0,2,-72
    CortexLeft:                 29696 out of 32492 vertices
    CortexRight:                29716 out of 32492 vertices

In those BOLD dtseries.nii files the values of the medial wall grayordinates are all zeroes though.

@mgxd
Copy link
Collaborator Author

mgxd commented May 6, 2022

Ok, thank you for the diligence in getting this in shape! This just goes to show how much a regression test on the CIFTI outputs is needed.

Now I am not sure if we ever were producing HCP compatible CIFTIs (well, at least since porting over the dcan-infant-pipeline alignment)...

I've pushed a new patch, I've tested it locally and everything looks good, but will hold off on merging this until you confirm.

@mgxd
Copy link
Collaborator Author

mgxd commented May 6, 2022

BTW, @madisoth do you want to add yourself to the zenodo - if so, feel free to open a PR

@mgxd mgxd merged commit 90e392e into nipreps:master May 11, 2022
@mgxd mgxd deleted the fix/cifti-output branch May 11, 2022 18:05
@mgxd mgxd added this to the 22.0.2 milestone May 11, 2022
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