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

Nilearn-style reporting (Issue #341) #342

Merged
merged 30 commits into from
Mar 24, 2021
Merged

Nilearn-style reporting (Issue #341) #342

merged 30 commits into from
Mar 24, 2021

Conversation

man-shu
Copy link
Contributor

@man-shu man-shu commented Jan 19, 2021

No description provided.

@bthirion
Copy link
Member

Thx for opening the PR !
Please ping me whenever you need.

@man-shu
Copy link
Contributor Author

man-shu commented Jan 20, 2021

Great! Will do.

pypreprocess/reporting/check_preprocessing.py Outdated Show resolved Hide resolved
pypreprocess/nipype_preproc_spm_utils.py Outdated Show resolved Hide resolved
pypreprocess/nipype_preproc_spm_utils.py Outdated Show resolved Hide resolved
pypreprocess/nipype_preproc_spm_utils.py Outdated Show resolved Hide resolved
pypreprocess/nipype_preproc_spm_utils.py Outdated Show resolved Hide resolved
pypreprocess/nipype_preproc_spm_utils.py Show resolved Hide resolved
pypreprocess/nipype_preproc_spm_utils.py Outdated Show resolved Hide resolved
pypreprocess/reporting/check_preprocessing.py Outdated Show resolved Hide resolved
pypreprocess/reporting/preproc_reporter.py Outdated Show resolved Hide resolved
pypreprocess/subject_data.py Outdated Show resolved Hide resolved
@bthirion
Copy link
Member

@man-shu please let me know when this is ready for review.

@man-shu
Copy link
Contributor Author

man-shu commented Feb 24, 2021

Yes @bthirion, will do!

@bthirion
Copy link
Member

Hi @man-shu , Can you fix the CI and then let me know ?
Best,

@man-shu
Copy link
Contributor Author

man-shu commented Mar 15, 2021

Yes, on it!

@man-shu man-shu force-pushed the nilearn-reporting branch from 32c6f1e to 6f94ebb Compare March 17, 2021 12:11
@man-shu
Copy link
Contributor Author

man-shu commented Mar 17, 2021

Hello @bthirion,
Except for the docstrings, this branch is now ready for review.

Copy link
Member

@bthirion bthirion left a comment

Choose a reason for hiding this comment

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

Thx ! I just put some minro comments.

May main question is: do we have at least a smoke test that runs the reports ?

pypreprocess/reporting/check_preprocessing.py Outdated Show resolved Hide resolved
pypreprocess/reporting/pypreproc_reporter.py Outdated Show resolved Hide resolved
pypreprocess/reporting/pypreproc_reporter.py Outdated Show resolved Hide resolved
pypreprocess/reporting/pypreproc_reporter.py Outdated Show resolved Hide resolved
pypreprocess/reporting/pypreproc_reporter.py Outdated Show resolved Hide resolved
pypreprocess/reporting/pypreproc_reporter.py Outdated Show resolved Hide resolved
pypreprocess/reporting/pypreproc_reporter.py Outdated Show resolved Hide resolved
pypreprocess/reporting/pypreproc_reporter.py Outdated Show resolved Hide resolved
@man-shu
Copy link
Contributor Author

man-shu commented Mar 22, 2021

Thx ! I just put some minro comments.

May main question is: do we have at least a smoke test that runs the reports ?

Thanks, @bthirion for the review and comments. The tests run by the Circle CI generate the reports with the new reporting, I checked the log and it does seem to be working. It prints a line "Report created and saved to - " when a report is created. But I am not sure if that qualifies as a smoke test. Let me know if a more rigorous test is needed (I might need to consult someone, maybe Nicolas, on how to write one).

@bthirion
Copy link
Member

OK, so indeed a test should be run, at least to make sure that documentation generation runs.

But: I propose to defer it to a frtcoming PR. Please address the minro comments, then we merge and add a test next.

Copy link
Member

@bthirion bthirion left a comment

Choose a reason for hiding this comment

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

LGTM, thx !

@man-shu
Copy link
Contributor Author

man-shu commented Mar 24, 2021

Cool! merging...

@man-shu man-shu merged commit 7ad460d into master Mar 24, 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.

2 participants