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

ENH: Switch to the refactored report generation from NiWorkflows #1599

Merged
merged 10 commits into from
May 2, 2019

Conversation

oesteban
Copy link
Member

@oesteban oesteban commented May 1, 2019

In preparation for #1596 and #1586, this PR fully migrates fMRIPrep's generation of reports to NiWorkflows.

A side work stripping out some sentry tracking of errors from the generation function was necessary. The idea would be to keep cleaning up the run.py script from sentry tracking by moving more functions to fmriprep/utils/sentry.py.

@oesteban oesteban requested a review from effigies May 1, 2019 16:27
@oesteban
Copy link
Member Author

oesteban commented May 1, 2019

This PR splits some of the changes of #1596, so that one can be reviewed after this is merged to only show changes that pertain to the new --output-spaces argument.

@effigies
Copy link
Member

effigies commented May 1, 2019

I assume a CircleCI build should show up?

@oesteban
Copy link
Member Author

oesteban commented May 1, 2019

@effigies
Copy link
Member

effigies commented May 1, 2019

Cool. I'll wait on that to review.

@oesteban
Copy link
Member Author

oesteban commented May 1, 2019

Okay, this seems to be getting closer. Please notice that ds005 is generating a T1w-to-MNI152NLin6Asym that is not supposed to appear. I think it comes from ICA-AROMA? I'll let you know when I find out.

When ``$HOME`` was changed to ``/home/fmriprep`` for Docker images I
missed updating the mount point of the wrapper.

This PR addresses that issue.
@effigies
Copy link
Member

effigies commented May 2, 2019

@oesteban I can't restart the failed job because it's under your user in Circle.

@oesteban
Copy link
Member Author

oesteban commented May 2, 2019

I'll try to get back at this. I found out that it is 99% sure an old reportlet perpetuating under work/reportlets. I've cleaned up the cache and now I delete the reportlets folder after the anatomical workflow.

However, the job would not run now. I think in part because of #1602 as the anatomical work dir is not being reused.

@oesteban
Copy link
Member Author

oesteban commented May 2, 2019

Actually, I'm wrong: in the cleanup I mistakenly included the freesurfer dir for deletion. Fixing that now.

@oesteban
Copy link
Member Author

oesteban commented May 2, 2019

Thanks for #1602, this build should be the one.

@oesteban
Copy link
Member Author

oesteban commented May 2, 2019

@effigies green lights! at last!

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.

Looks fine. So niworkflows is now hosting fMRIPrep's report configuration YAML? It seems like it makes more sense here.

@oesteban
Copy link
Member Author

oesteban commented May 2, 2019

I agree we will eventually want to get the control over the config file back to fMRIPrep. However, I would keep the fmriprep.yml file as the base exemplar in Niworkflows.

Given that such a config file does not change a lot through history, I'd say to keep it in niworkflows for now and move it over here when necessary.

In theory, generate_report should keep working with user-supplied config files. That is not currently tested. What I'm trying to say is that everything should be in place for us to move the config file here when we want.

I'll go ahead and merge this in.

@oesteban oesteban merged commit eef07cd into nipreps:master May 2, 2019
@oesteban oesteban deleted the enh/new-reports branch May 2, 2019 20:45
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.

None yet

2 participants