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

Add generation of scene/snapshots for fMRIQC #203

Merged
merged 17 commits into from
Jan 11, 2021
Merged

Add generation of scene/snapshots for fMRIQC #203

merged 17 commits into from
Jan 11, 2021

Conversation

mharms
Copy link
Contributor

@mharms mharms commented Dec 23, 2020

Still need to revise and add the scene template itself, but wanted to get the code up for initial review.

Of note, the bias corrected, normalized, but not brain masked version of the scout is named with a "_notmasked" suffix currently. "unmasked" was a bit too cryptic in my opinion, although open to other suggestions.

@coalsont
Copy link
Member

_nomask could be a shorter but equivalent specifier, I like shorter when it doesn't sacrifice clarity. For brainstorming, "with background" is a different way to think about it, but _withbg doesn't look as clear, and background is fairly long to put in a filename.

I think a few files use _brain for the masked version, and while _nobrain might be amusing, it is unlikely to be helpful.

@mharms
Copy link
Contributor Author

mharms commented Dec 23, 2020

_nomask seems like a good alternative.

Copy link
Contributor

@glasserm glasserm left a comment

Choose a reason for hiding this comment

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

The code looks good to me and thanks for making a no mask SBRef. I would like to look over some test results prior to approving.

@mharms
Copy link
Contributor Author

mharms commented Jan 6, 2021

With the above additions, this should now be operational. I'm running another test overnight (denovo execution of fMRIVolume and fMRISurface) and can generate a -zip-scene-file for sharing tomorrow.

@mharms
Copy link
Contributor Author

mharms commented Jan 7, 2021

The code as it exists in this PR works. I'll email you the location of the scene file.

@mharms
Copy link
Contributor Author

mharms commented Jan 8, 2021

@glasserm : It would be great if you can review the scene file. Would like to get this PR into the master, tag a new 4.3rc, and then have QuNex folks build a new container using that.

@glasserm
Copy link
Contributor

glasserm commented Jan 9, 2021

@glasserm : It would be great if you can review the scene file. Would like to get this PR into the master, tag a new 4.3rc, and then have QuNex folks build a new container using that.

I looked but did not find an e-mail from you showing the location of the scene file.

@mharms
Copy link
Contributor Author

mharms commented Jan 9, 2021

@glasserm I sent the email again.

@glasserm glasserm self-requested a review January 9, 2021 19:39
@mharms
Copy link
Contributor Author

mharms commented Jan 11, 2021

@coalsont: Here's the git diff on the approach that @hodgem took for the set -e issue:

 main()
 {
+
        log_Msg "Starting main functionality"
+       set -e
 
        # Retrieve positional parameters
        local StudyFolder="${1}"
@@ -1045,7 +1047,9 @@ main()
         fi
     done
 
+       set +e
        log_Msg "Completing main functionality"
+
 }

He confirmed that this does indeed catch the "error" when wb_command got killed due to insufficient memory. Although your coupling its placement to the debug.shlib line seems cleaner, if it doesn't need to be inside main()

@mharms
Copy link
Contributor Author

mharms commented Jan 11, 2021

Just for public documentation purposes, the set -e issue mentioned above it completely unrelated to this PR, and relates to something we need to tweak in DeDriftAndResample

@coalsont
Copy link
Member

The set -e should be done immediately before sourcing debug.shlib, so that it can disable it for interactive shells, substantially reducing a considerable annoyance when doing bash debugging (unexpectedly losing the entire shell, with all carefully set up script arguments, when set -e trips).

@coalsont coalsont merged commit 63f37cf into master Jan 11, 2021
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