RF: Load report assembler from nireports #3177

celprov · 2023-12-07T23:14:36Z

This transition is inspired by how MRIQC defines generate_reports using nireports' Report()

mnt: load Report from nireports instead of niworkflows
mnt: adapt function arguments accordingly
mnt: delete run_reports function
mnt: delete Report patch subclass that is no longer necessary

celprov · 2023-12-07T23:45:18Z

@oesteban two problems:

How do give layout.get() not only the subject label but also the file path? or how do I construct entities differently so that it indicates the subject_label but also all the other info it needs to indicate?
What should I do about those lines given that generate_report() always return 0 ?

effigies · 2023-12-08T20:00:52Z

If it's not a drop-in replacement, let's postpone this until the next release.

codecov · 2023-12-13T00:44:12Z

Codecov Report

Attention: 20 lines in your changes are missing coverage. Please review.

Comparison is base (b248ae6) 72.35% compared to head (89da33b) 72.03%.
Report is 18 commits behind head on master.

Files	Patch %	Lines
fmriprep/reports/core.py	13.33%	13 Missing ⚠️
fmriprep/cli/workflow.py	0.00%	4 Missing ⚠️
fmriprep/cli/run.py	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3177      +/-   ##
==========================================
- Coverage   72.35%   72.03%   -0.33%     
==========================================
  Files          55       55              
  Lines        4102     4101       -1     
==========================================
- Hits         2968     2954      -14     
- Misses       1134     1147      +13

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

celprov · 2023-12-13T00:52:05Z

I think the failure in Contribution checks is triggered by the fact that the colorama python package is not installed in the CI, because I ran python -m black --diff --color --check fmriprep locally and nothing changed.

Secondly, what should I do about the ModuleNotFoundError "pkg_resources" that triggers the failure of the other tests?

effigies · 2023-12-13T01:46:27Z

Yes, the isort problem is waiting on a new release. We can ignore it for now.

It looks like nireports requires pkg_resources, which is distributed with setuptools. We should not add it as a runtime dependency, we should purge it in favor of importlib.resources. I'll open a PR over there.

effigies · 2023-12-13T03:59:48Z

pkg_resources fix: nipreps/nireports#85

maint: adapt function arguments accordingly maint: delete run_reports function

…om:celprov/fmriprep into maint/load-report-assembler-from-nireports

fmriprep/reports/core.py

fix: fix key name in entities dict

celprov · 2023-12-14T01:55:09Z

The test now passes, and the report is correctly generated.
I'm not entirely sure about how errors related to the failure in report generation are handled. Feedback on that part is more than welcome.

fmriprep/reports/core.py

oesteban · 2023-12-18T10:41:45Z

If it's not a drop-in replacement, let's postpone this until the next release.

Does this mean "please do not merge into master just yet"?

celprov · 2024-01-17T09:53:57Z

I corrected how the error generated by rob.generate_reports() is logged following the Mathias' suggestion.
I also opened issue #3207 so we keep in mind that counting the number of crash files needs to be implemented.
I think this is related to the test that is not successful.

@oesteban @effigies, please review this PR, let me know if I'm missing anything, or merge if all is good.
Thanks!

effigies

Two suggestions, kind of interleaved.

It feels cleaner just to track the failing subjects as a list, and build the string at the end.
We have our global logger config, so let's use it.

fmriprep/reports/core.py

fmriprep/cli/run.py

Co-authored-by: Chris Markiewicz <[email protected]> Co-authored-by: Oscar Esteban <[email protected]>

fmriprep/reports/core.py

…te_reports

…essage

celprov · 2024-01-18T10:17:11Z

@oesteban @effigies I've applied your suggestions; let me know what you think.

effigies · 2024-01-18T14:30:52Z

We need to fix the ds054 tests ASAP, but we agreed that would be outside this PR.

…ly sampled dataset (#3191) enh: beyond a certain number of session for a single subject (4), we separate the anatomical report from the functional and we separate the functional reports per session. For fewer sessions than 4, we keep the original behavior of aggregating all sessions and the anatomical in one report. enh: find the suitable bootstrapfile within generate_reports rather than before enh: add session_list argument to generate_reports Builds on top of #3177 Closes #3080 --------- Co-authored-by: Oscar Esteban <[email protected]> Co-authored-by: Chris Markiewicz <[email protected]>

…ly sampled dataset (nipreps#3191) enh: beyond a certain number of session for a single subject (4), we separate the anatomical report from the functional and we separate the functional reports per session. For fewer sessions than 4, we keep the original behavior of aggregating all sessions and the anatomical in one report. enh: find the suitable bootstrapfile within generate_reports rather than before enh: add session_list argument to generate_reports Builds on top of nipreps#3177 Closes nipreps#3080 --------- Co-authored-by: Oscar Esteban <[email protected]> Co-authored-by: Chris Markiewicz <[email protected]>

24.0.0 (June 17, 2024) New feature release in the 24.0.x series. This release is an incremental improvement on 23.2.x, with some fixes for bugs discovered in the updated workflow. New features include separation of HTML reports by session for subjects with many BOLD runs, a new ``--fs-no-resume`` option to improve interoperability with less typical FreeSurfer directories, such as those generated by longitudinal FreeSurfer or FastSurfer, and adoption of DatasetLinks and BIDS-URIs, to follow the recommendations of recent versions of BIDS. With thanks to Dimitri Papadopoulos, Basile Pinsard, Celine Provins, Taylor Salo and Wang Hao-Ting for their contributions! * FIX: Add "double" type to allowed DisplacementFieldTransform (#3287) * FIX: Require recent templateflow, select correct aparc dseg.tsv (#3256) * FIX: Ensure proper templates are retrieved with sloppy (#3251) * FIX: Delete summary from functional report when separated by sessions (#3223) * FIX: Support lists in bids filter file containing ``null`` or ``*`` (#3215) * FIX: Re-enable anat fasttrack for dataset without t1w (#3202) * ENH: Use BIDSURI in init_ds_boldmask_wf (#3297) * ENH: Add templateflow to DatasetLinks (#3267) * ENH: Track proximal sources of functional GIFTIs (#3263) * ENH: Support named derivative paths (#3264) * ENH: Track Sources for standard-space outputs (#3262) * ENH: Add --fs-no-resume option to reuse existing FreeSurfer outputs without resuming (#3142) * ENH: Use BIDS URIs to track Sources in sidecars (#3255) * ENH: Ignore unselected subjects in BIDSLayoutIndexer (#3236) * ENH: Add metadata for motion parameters (#3245) * ENH: Separate anatomical and functional reports per session for densely sampled dataset (#3191) * ENH: Leverage T2w if available for BOLD -> anat coregistration (#3208) * RF: Fix ITK warp conversion to nitransforms format (#3300) * RF: Load report assembler from nireports (#3177) * DOC: Clarify ``--dvars-spike-threshold`` uses standardized DVARS (#3205) * TST: Update test to reflect new report generation behavior (#3210) * STY: Manual conversions to f-strings (#3241) * STY: Apply ruff/pyupgrade rule UP031 (#3280) * STY: Lint and style check full repository (#3221) * STY: Adopt ruff for linting and formatting (#3206) * MNT: Pin libitk 5.3 and note dependencies (#3298) * MNT: Upgrade ruff pre-commit, add fixing checks (#3283) * MNT: Complete transition from flake8/black to ruff (#3279) * MNT: Apply Repo-Review suggestions (#3194) * MNT: Verbatim copy of Apache license 2.0 (#3259) * MNT: Bump cryptography from 41.0.7 to 42.0.4 (#3234) * MNT: Drop copyright year, unused dunder fields (#3247) * MNT: Update environment pins (#3226) * MNT: Bump codecov/codecov-action from 3 to 4 (#3219) * DOCKER: Restore mincinfo binary (#3249) * CI: Move to new circle machine tags (#3248) * CI: Avoid ruff warning (#3244) * CI: Pass ruff tests (#3243)

effigies added this to the 24.0.0 milestone Dec 8, 2023

celprov force-pushed the maint/load-report-assembler-from-nireports branch from 60a04df to eec1902 Compare December 13, 2023 00:28

celprov force-pushed the maint/load-report-assembler-from-nireports branch from 2ede5fc to eec1902 Compare December 13, 2023 00:51

effigies mentioned this pull request Dec 13, 2023

MNT: Load reportlets interfaces from nireports rather than niworkflows #3184

Merged

celprov added 2 commits December 13, 2023 13:46

maint: load Report from nireports instead of niworkflows

1be8fcc

maint: adapt function arguments accordingly maint: delete run_reports function

enh: adapt the call to Report to its nireports implementation

f675648

effigies force-pushed the maint/load-report-assembler-from-nireports branch from eec1902 to f675648 Compare December 13, 2023 18:46

celprov added 4 commits December 13, 2023 12:00

maint: load Report from nireports instead of niworkflows

363b9f3

maint: adapt function arguments accordingly maint: delete run_reports function

enh: adapt the call to Report to its nireports implementation

f0606af

Merge branch 'maint/load-report-assembler-from-nireports' of github.c…

a690eba

…om:celprov/fmriprep into maint/load-report-assembler-from-nireports

fix: remove deprecated packagename argument of generate_reports

54d4366

effigies reviewed Dec 13, 2023

View reviewed changes

fmriprep/reports/core.py Outdated Show resolved Hide resolved

celprov added 3 commits December 13, 2023 13:45

fix: delete Report patch subclass as it is not useful anymore

7ab02a0

sty: format to PEP8

ee9f547

fix: fix key name in entities dict

fix: bootstrap_file argument

6756741

celprov marked this pull request as ready for review December 14, 2023 01:51

celprov requested a review from effigies December 14, 2023 01:52

celprov changed the title ~~WIP: load report assembler from nireports~~ ENH: load report assembler from nireports Dec 14, 2023

celprov changed the title ~~ENH: load report assembler from nireports~~ MNT: load report assembler from nireports Dec 14, 2023

effigies reviewed Dec 14, 2023

View reviewed changes

fmriprep/reports/core.py Outdated Show resolved Hide resolved

fix: remove deprecated packagename argument of generate_reports

42ec59f

celprov force-pushed the maint/load-report-assembler-from-nireports branch from 0e36362 to 42ec59f Compare December 16, 2023 00:02

celprov mentioned this pull request Dec 21, 2023

ENH: separate anatomical and functional reports per session for densely sampled dataset #3191

Merged

This was referenced Jan 15, 2024

Running fMRIPrep roadmap TheAxonLab/hcph-sops#417

Open

Separate count of crash files from count of error in report generation #3207

Closed

fix: correct the logging of errors in report generation

e66fd05

celprov force-pushed the maint/load-report-assembler-from-nireports branch from fe255c5 to e66fd05 Compare January 17, 2024 09:47

effigies reviewed Jan 17, 2024

View reviewed changes

oesteban reviewed Jan 18, 2024

View reviewed changes

fmriprep/reports/core.py Outdated Show resolved Hide resolved

fmriprep/reports/core.py Outdated Show resolved Hide resolved

fmriprep/reports/core.py Show resolved Hide resolved

fmriprep/cli/run.py Show resolved Hide resolved

Apply suggestions from code review

8692b13

Co-authored-by: Chris Markiewicz <[email protected]> Co-authored-by: Oscar Esteban <[email protected]>

oesteban reviewed Jan 18, 2024

View reviewed changes

fmriprep/reports/core.py Show resolved Hide resolved

celprov added 2 commits January 18, 2024 11:08

fix: report report generation report in the cli rather than in genera…

b33c91e

…te_reports

fix: both sentry and config.loggers are needed to capture the error m…

89da33b

…essage

celprov requested review from effigies and oesteban January 18, 2024 10:17

oesteban approved these changes Jan 18, 2024

View reviewed changes

effigies changed the title ~~MNT: load report assembler from nireports~~ RF: Load report assembler from nireports Jan 18, 2024

effigies approved these changes Jan 18, 2024

View reviewed changes

effigies merged commit 9568ccb into nipreps:master Jan 18, 2024
14 of 17 checks passed

celprov mentioned this pull request Jan 18, 2024

The test for crash file count on ds054 requires fixing. #3209

Closed

celprov deleted the maint/load-report-assembler-from-nireports branch January 18, 2024 16:16

oesteban mentioned this pull request Jan 19, 2024

Issues with reports after migrating to NiReports #3211

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RF: Load report assembler from nireports #3177

RF: Load report assembler from nireports #3177

celprov commented Dec 7, 2023 •

edited

Loading

celprov commented Dec 7, 2023

effigies commented Dec 8, 2023

codecov bot commented Dec 13, 2023 •

edited

Loading

celprov commented Dec 13, 2023 •

edited

Loading

effigies commented Dec 13, 2023

effigies commented Dec 13, 2023

celprov commented Dec 14, 2023

oesteban commented Dec 18, 2023

celprov commented Jan 17, 2024 •

edited

Loading

effigies left a comment

celprov commented Jan 18, 2024

effigies commented Jan 18, 2024

RF: Load report assembler from nireports #3177

RF: Load report assembler from nireports #3177

Conversation

celprov commented Dec 7, 2023 • edited Loading

celprov commented Dec 7, 2023

effigies commented Dec 8, 2023

codecov bot commented Dec 13, 2023 • edited Loading

Codecov Report

celprov commented Dec 13, 2023 • edited Loading

effigies commented Dec 13, 2023

effigies commented Dec 13, 2023

celprov commented Dec 14, 2023

oesteban commented Dec 18, 2023

celprov commented Jan 17, 2024 • edited Loading

effigies left a comment

Choose a reason for hiding this comment

celprov commented Jan 18, 2024

effigies commented Jan 18, 2024

celprov commented Dec 7, 2023 •

edited

Loading

codecov bot commented Dec 13, 2023 •

edited

Loading

celprov commented Dec 13, 2023 •

edited

Loading

celprov commented Jan 17, 2024 •

edited

Loading