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

Naming of html reports and aroma regressors #8

Open
burdinskid13 opened this issue Nov 4, 2022 · 3 comments
Open

Naming of html reports and aroma regressors #8

burdinskid13 opened this issue Nov 4, 2022 · 3 comments

Comments

@burdinskid13
Copy link

What would you like to see added in fMRIPrep?

html report naming:
For my use case, I need to run fmriprep on two sessions separately. i'd like to be able to merge the outputs into the same derivatives directory afterwards. however, since the html reports in the new layout (eg. in v22) are above the ses-* directory, i would need to rename the html reports in order to not have two files named the same in the same location. it would be great if the html report naming could include ses-* in addition to sub-*.

ICA aroma regressor naming:
I noticed that the naming of ICA aroma regressors in *_desc-confounds_timeseries.json and *_desc-confounds_timeseries.tsv is inconsistent. Naming the two regressors the same would make workflows just a little smoother. For example, the first motion regressor in the json is called "aroma_motion_1" but in the tsv it's called " aroma_motion_01". i got this with v22.0.2.

Do you have any interest in helping implement the feature?

Yes, but I would need guidance

Additional information / screenshots

No response

@effigies
Copy link
Member

effigies commented Dec 3, 2022

Hi @burdinskid13, thanks for your interest. I'll answer both here, but please open a separate bug report for the ICA AROMA one, and I'll copy my response over there.

For the HTML report issue, I wonder if the quicker fix is running --reports-only on the combined derivatives directory after running the two sessions separately. I haven't tried this. You'll also need to use the patch in nipreps/fmriprep#2900 to correctly handle this if you're using --output-layout bids (or not using --output-layout legacy in 21.0+). For this, you can use fmriprep-docker <usual arguments> --patch fmriprep=$PWD/fmriprep if you're running from inside the repository.

If that doesn't work/appeal, adding the session is possible. You would need to modify niworkflows here. We would need to accept a session and optionally add it to the filename. Then in fMRIPrep, we would need to detect that session has a unique value and pass that to niworkflows. Look for the function generate_reports.


For the ICA AROMA regressor naming, to address this, we'll need to look at where they come from. Looks like the ica_aroma_wf has the following output:

https://github.com/nipreps/fmriprep/blob/b6c8e1bfd2dd153663ac6bc17ec905ddf1e72b11/fmriprep/workflows/bold/confounds.py#L832-L833

That's set here:

https://github.com/nipreps/fmriprep/blob/b6c8e1bfd2dd153663ac6bc17ec905ddf1e72b11/fmriprep/workflows/bold/confounds.py#L1005-L1007

So generated by:

https://github.com/nipreps/fmriprep/blob/b6c8e1bfd2dd153663ac6bc17ec905ddf1e72b11/fmriprep/workflows/bold/confounds.py#L939-L941

So we'll be looking in:

https://github.com/nipreps/fmriprep/blob/b6c8e1bfd2dd153663ac6bc17ec905ddf1e72b11/fmriprep/interfaces/confounds.py#L262-L296

Which calls:

https://github.com/nipreps/fmriprep/blob/b6c8e1bfd2dd153663ac6bc17ec905ddf1e72b11/fmriprep/interfaces/confounds.py#L398-L463

It looks like the metadata and columns are defined in two places:

https://github.com/nipreps/fmriprep/blob/b6c8e1bfd2dd153663ac6bc17ec905ddf1e72b11/fmriprep/interfaces/confounds.py#L430-L432

https://github.com/nipreps/fmriprep/blob/b6c8e1bfd2dd153663ac6bc17ec905ddf1e72b11/fmriprep/interfaces/confounds.py#L459-L461

The fix may be simply to change {} to {:02d}. Would need to test, however, since if it's not an integer type, we'll need to convert name to an integer to do that formatting.

@tsalo
Copy link
Collaborator

tsalo commented Jun 15, 2023

Given that AROMA was removed in 23.1.0, are there any elements in this issue that should still be implemented?

@effigies
Copy link
Member

I think it can be closed. But worth opening on nipreps/fmripost-aroma...

@effigies effigies transferred this issue from nipreps/fmriprep Dec 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Todo
Development

No branches or pull requests

3 participants