[WIP] ENH: Add --func-only option to skip anatomical processing #808

madeleinesnyder · 2017-11-01T16:02:29Z

Connect this workflow or, probably, return it instead of wf.
Fill up the structure of the new workflow init_onlyfunc_preproc_wf
Write func-only alternatives to the Summary node and the Reports node, and connect them.
Verify your name, affiliation and insert your ORCID (if available) in the credits file.

…dismiss-t1-patch

Sync with refactored bold module

madeleinesnyder · 2017-11-01T16:39:09Z

Ah yes, got it. I see now there is a whole new workflow to take care of the func-only case instead of editing the original func workflow.

Also, I was thinking that it might be helpful for me to contribute a BIDSgenerator file that would be easily configured by people who want to use the pipeline but don't have the BIDS data format. I know our lab was hesitant to use the pipeline because of the overhead of renameing and restructuring data, so it might increase use if there is an add-on file that takes care of this. I could put in the places in run.py and other scripts where BIDSgenerator.py is called

oesteban · 2017-11-01T16:40:48Z

Sure, but it is a lot easier to keep things focused :). Feel free to open as many PRs as features you'd like to contribute. It's much more manageable when contributions are handled separatelly

oesteban · 2017-11-01T16:46:43Z

As you can see above, I just pushed one commit to your branch. You can see it where it says "convert indentation to spaces" above.

To get those changes, make sure you git pull on your repo after you make sure you are in the correct branch with git checkout func-only.

madeleinesnyder · 2017-11-01T16:47:37Z

Ok, so I don't mess this up:

To open a PR to push the BIDSgenerator.py file and other files updated with (commented out) lines to handle this script, I would do a new PR from the https://github.com/poldracklab/fmriprep page on the master branch?

oesteban · 2017-11-01T17:04:00Z

Yes - to open a new PR:

Make sure you are in sync with the upstream repo:

git fetch upstream
git checkout master
git merge upstream/master

Open up a new branch

git checkout -b new-feature-branch

Edit the code (add changes now), commit changes and push:

<edit files>
git add <new files that were not in the repo already>
git commit -a
git push --set-upstream origin new-feature-branch

Go to GitHub.com and request a new PR
Repeat 3 until the code is acceptable to be merged in.

madeleinesnyder · 2017-11-01T17:18:03Z

Alrighty I just requested a new one-- it should be just two files, one added (BIDSgenerator.py) and one modified (run.py)

oesteban · 2017-11-01T17:34:05Z

Cool!

Let's focus on getting this one merged first 👍

I have updated your PR comment up above with the tasks that need be done before merging.

You will also see below my comment a message saying "All checks have failed". This means that there are problems with the code and it didn't pass tests. Let's not worry about this now, but tests will need to get green before we can merge. We'll address this in a future.

Please let me know what is stopping you at this point (if any).

madeleinesnyder · 2017-11-01T17:51:33Z

Ok sounds good. I just connected up the onlyfunc workflow in base.py and I need to know what to merge: I did git add --all git commit -m "[my message]" now what? (git push --set-upstream origin master?)

…

On Wed, Nov 1, 2017 at 10:34 AM, Oscar Esteban ***@***.***> wrote: Cool! Let's focus on getting this one merged first 👍 I have updated your PR comment up above with the tasks that need be done before merging. You will also see below my comment a message saying "All checks have failed". This means that there are problems with the code and it didn't pass tests. Let's not worry about this now, but tests will need to get green before we can merge. We'll address this in a future. Please let me know what is stopping you at this point (if any). — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#808 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AJctLUepIenlJEV2NAz2vfJeiMOL2P2rks5syKuOgaJpZM4QOaz_> .

oesteban · 2017-11-01T17:53:57Z

git push

The --set-upstream origin is only for the first time you push. After that origin is always set when you push.

madeleinesnyder · 2017-11-01T17:55:44Z

ok done

…

On Wed, Nov 1, 2017 at 10:53 AM, Oscar Esteban ***@***.***> wrote: git push The --set-upstream origin is only for the first time you push. After that origin is always set when you push. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#808 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AJctLXn65-zj2tXhAxLO44IdsK-Mbzsbks5syLA2gaJpZM4QOaz_> .

oesteban · 2017-11-01T18:00:05Z

Indeed!

I can see your changes here: 940340b

BTW, with the git add --all you also included a file called fmriprep/workflows/.base.py.swo (vim's temporary files).

Please remove it:

git rm fmriprep/workflows/.base.py.swo
git commit -am 'remove fmriprep/workflows/.base.py.swo'
git push

effigies

Just a couple quick things I noticed. This is looking good!

effigies · 2017-11-01T18:01:23Z

.zenodo.json

+      "name": "Madeleine Snyder",
+      "affiliation": "Harvard Medical School, Harvard University, Cambridge, Massachusetts"
+    },
+    {


Could you move this to just above Russ Poldrack? Outside contributors are listed in order of contribution, starting at Feilong Ma.

Also, we haven't generally included city and state in the affiliation. For consistency, I'd suggest dropping it. And do you have an ORCID? It doesn't take much time to get one, and they help disambiguate researchers with similar names, as well as persist across changes in institution/name.

Sorry for the ordering issue, it's my bad. @madeleinesnyder please fix the ordering and add your ORCID if available/possible.

effigies · 2017-11-01T18:11:43Z

fmriprep/workflows/base.py

+        ])
+
+        if anat_only:
+            return workflow


If I specify --anat-only and --func-only, this won't be hit, so the functional workflows will still be generated. There should probably at some point (possibly in run.py) be a check to raise an exception if both flags are passed to fmriprep.

Ok, I would just put in a simple if anat_only and func_only raise exception before any of the workflows are put together. Any reasons to put it elsewhere?

The "proper" way to do this is using mutually exclusive options

# Create a mutually exclusive group for --anat-only and --func-only g_blocks = parser.add_mutually_exclusive_group() g_blocks.add_argument('--anat-only', action='store_true', help='run anatomical workflows only') g_blocks.add_argument('--func-only', action='store_true', help='run functional workflows only ' '(dismiss the anatomical images if present).')

…ously

oesteban · 2017-11-13T19:28:57Z

fmriprep/workflows/base.py

+            workflow.connect([
+                (inputnode, func_preproc_wf, [('subjects_dir', 'inputnode.subjects_dir')]),
+                (bidssrc, bids_info, [
+                    (('bold', fix_multi_bold_source_name), 'in_file')]),


fix_multi_bold_source_name is undefined

I assumed there was an analog to fic_multi_t1w_source_name, which pops up in the anat_only workflow.

oesteban · 2017-11-13T19:29:11Z

fmriprep/workflows/base.py

+                (bidssrc, func_preproc_wf, [('bold', 'inputnode.bold')]),
+                (summary, func_preproc_wf, [('subject_id', 'inputnode.subject_id')]),
+                (bidssrc, ds_summary_report, [
+                    (('bold', fix_multi_bold_source_name), 'source_file')]),


fix_multi_bold_source_name is undefined

oesteban · 2017-11-13T19:29:19Z

fmriprep/workflows/base.py

+                    (('bold', fix_multi_bold_source_name), 'source_file')]),
+                (summary, ds_summary_report, [('out_report', 'in_file')]),
+                (bidssrc, ds_about_report, [
+                    (('bold', fix_multi_bold_source_name), 'source_file')]),


fix_multi_bold_source_name is undefined

oesteban · 2017-11-13T19:32:06Z

I can put some hours on pushing this PR forward this week, WDYT @madeleinesnyder ?

There are some outstanding errors, if you look at Travis tests:

$ flake8 fmriprep wrapper
fmriprep/workflows/base.py:482:31: F821 undefined name 'fix_multi_bold_source_name'
fmriprep/workflows/base.py:489:31: F821 undefined name 'fix_multi_bold_source_name'
fmriprep/workflows/base.py:492:31: F821 undefined name 'fix_multi_bold_source_name'
fmriprep/workflows/bold/base.py:751:9: F841 local variable 'bold_pe' is assigned to but never used
fmriprep/workflows/bold/base.py:772:5: E303 too many blank lines (2)
fmriprep/workflows/bold/base.py:779:9: F841 local variable 'bold_stc_wf' is assigned to but never used
fmriprep/workflows/bold/base.py:788:9: F821 undefined name 'summary'
fmriprep/workflows/bold/base.py:825:29: F821 undefined name 'func_reports_wf'
fmriprep/workflows/bold/base.py:843:9: E303 too many blank lines (2)
fmriprep/workflows/bold/base.py:845:9: E122 continuation line missing indentation or outdented
fmriprep/workflows/bold/base.py:846:9: E122 continuation line missing indentation or outdented
fmriprep/workflows/bold/base.py:848:9: E122 continuation line missing indentation or outdented
fmriprep/workflows/bold/base.py:848:23: F821 undefined name 'bold_confounds_wf'
fmriprep/workflows/bold/base.py:848:97: W291 trailing whitespace
fmriprep/workflows/bold/base.py:849:9: E122 continuation line missing indentation or outdented
fmriprep/workflows/bold/base.py:849:29: F821 undefined name 'func_reports_wf'
fmriprep/workflows/bold/base.py:851:9: E122 continuation line missing indentation or outdented
fmriprep/workflows/bold/base.py:851:10: F821 undefined name 'bold_confounds_wf'
fmriprep/workflows/bold/base.py:856:9: E122 continuation line missing indentation or outdented
fmriprep/workflows/bold/base.py:856:10: F821 undefined name 'bold_confounds_wf'
fmriprep/workflows/bold/base.py:856:29: F821 undefined name 'func_reports_wf'
fmriprep/workflows/bold/base.py:860:9: E122 continuation line missing indentation or outdented
fmriprep/workflows/bold/base.py:860:10: F821 undefined name 'bold_confounds_wf'
fmriprep/workflows/bold/base.py:860:29: F821 undefined name 'summary'
fmriprep/workflows/bold/base.py:861:9: E122 continuation line missing indentation or outdented
fmriprep/workflows/bold/base.py:861:10: F821 undefined name 'summary'
fmriprep/workflows/bold/base.py:861:19: F821 undefined name 'func_reports_wf'

madeleinesnyder · 2017-11-13T19:59:09Z

Yes, I can come work on main campus if that would help. Or work remotely from my lab. Either works. Let me know.

oesteban · 2017-12-07T17:21:57Z

Hey @madeleinesnyder, we released fmriprep-1.0.0 yesterday. I am hoping that after next week I'll have more time and can help you finish this up. Let me know when you can touch base.

madeleinesnyder · 2017-12-07T18:11:57Z

Congrats! My schedule is pretty flexible all next week, so let me know what time works best for you.

…

On Thu, Dec 7, 2017 at 9:21 AM, Oscar Esteban ***@***.***> wrote: Hey @madeleinesnyder <https://github.com/madeleinesnyder>, we released fmriprep-1.0.0 <https://github.com/poldracklab/fmriprep/releases/tag/1.0.0> yesterday. I am hoping that after next week I'll have more time and can help you finish this up. Let me know when you can touch base. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#808 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AJctLVWaYjYyJfjUTTV9rJEUv0DOE_45ks5s-B62gaJpZM4QOaz_> .

stale · 2018-10-03T18:30:06Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Madeleine Snyder and others added 7 commits October 16, 2017 13:44

ENH: Add --dismiss-t1w option to skip anatomical processing

a778b21

Merge remote-tracking branch 'upstream/master' into madeleine-snyder-…

b78a50d

…dismiss-t1-patch

[skip ci] Add Maddie to zenodo

16c367a

Merge pull request #2 from oesteban/madeleine-snyder-dismiss-t1-patch

cb3c6fc

Sync with refactored bold module

fix erroneously removed medial_surface_nan

9fcb693

fix erroneous initialization of anatomical workflow

db088e7

Merge branch 'master' into func-only

86c010e

oesteban mentioned this pull request Nov 1, 2017

[WIP] ENH: Add --dismiss-t1w option to skip anatomical processing #770

Closed

oesteban changed the title ~~Func only~~ [WIP] ENH: Add --func-only option to skip anatomical processing Nov 1, 2017

convert indentation to spaces

453e91f

Connected onlyfunc workflow in workflows/base.py

940340b

remove fmriprep/workflows/.base.py.swo

d9b66f6

effigies reviewed Nov 1, 2017

View reviewed changes

Madeleine Snyder added 3 commits November 1, 2017 11:39

Added raise exception if user tries func_only and anat_only simultane…

9b377ea

…ously

Connected other workflows in workflows/bold/base.py onlyfunc

d3decf9

Added mututally exclusive argument for func and anat in run.py

4878aae

oesteban mentioned this pull request Nov 2, 2017

[ENH] Efforts towards keeping memory low #807

Merged

5 tasks

oesteban added 2 commits November 13, 2017 11:21

remove .swo file, update .gitignore

fc7fd68

Merge remote-tracking branch 'upstream/master' into func-only

7403a97

fix zenodo ordering

5c6fb3d

oesteban reviewed Nov 13, 2017

View reviewed changes

Merge remote-tracking branch 'upstream/master' into func-only

1aab4c6

emdupre mentioned this pull request Dec 7, 2017

Workflow for multiecho data #884

Closed

oesteban mentioned this pull request May 9, 2018

Generate carpetplots even if MNI is not one of the output spaces #1119

Closed

oesteban mentioned this pull request Jul 19, 2018

All workflows require T1w images. Exception #1211

Closed

stale bot added the stale label Oct 3, 2018

stale bot closed this Oct 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] ENH: Add --func-only option to skip anatomical processing #808

[WIP] ENH: Add --func-only option to skip anatomical processing #808

madeleinesnyder commented Nov 1, 2017 •

edited

madeleinesnyder commented Nov 1, 2017

oesteban commented Nov 1, 2017

oesteban commented Nov 1, 2017

madeleinesnyder commented Nov 1, 2017

oesteban commented Nov 1, 2017

madeleinesnyder commented Nov 1, 2017

oesteban commented Nov 1, 2017

madeleinesnyder commented Nov 1, 2017 via email

oesteban commented Nov 1, 2017

madeleinesnyder commented Nov 1, 2017 via email

oesteban commented Nov 1, 2017

effigies left a comment

effigies Nov 1, 2017

oesteban Nov 1, 2017

effigies Nov 1, 2017

madeleinesnyder Nov 1, 2017

oesteban Nov 1, 2017

oesteban Nov 13, 2017

madeleinesnyder Nov 13, 2017

oesteban Nov 13, 2017

oesteban Nov 13, 2017

oesteban commented Nov 13, 2017

madeleinesnyder commented Nov 13, 2017

oesteban commented Dec 7, 2017

madeleinesnyder commented Dec 7, 2017 via email

stale bot commented Oct 3, 2018

[WIP] ENH: Add --func-only option to skip anatomical processing #808

[WIP] ENH: Add --func-only option to skip anatomical processing #808

Conversation

madeleinesnyder commented Nov 1, 2017 • edited

madeleinesnyder commented Nov 1, 2017

oesteban commented Nov 1, 2017

oesteban commented Nov 1, 2017

madeleinesnyder commented Nov 1, 2017

oesteban commented Nov 1, 2017

madeleinesnyder commented Nov 1, 2017

oesteban commented Nov 1, 2017

madeleinesnyder commented Nov 1, 2017 via email

oesteban commented Nov 1, 2017

madeleinesnyder commented Nov 1, 2017 via email

oesteban commented Nov 1, 2017

effigies left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oesteban commented Nov 13, 2017

madeleinesnyder commented Nov 13, 2017

oesteban commented Dec 7, 2017

madeleinesnyder commented Dec 7, 2017 via email

stale bot commented Oct 3, 2018

madeleinesnyder commented Nov 1, 2017 •

edited