Add prefix to all output files

[tsalo]

Closes #901 and closes #955.

Changes proposed in this pull request:

• Add the user-provided prefix to other outputs, including the text report, the HTML report, and figures.

[tsalo]

It looks like the figure filenames are hardcoded into the HTML templates, so the prefixes are tough to account for.

[eurunuela]

It looks like the figure filenames are hardcoded into the HTML templates, so the prefixes are tough to account for.

I could have a look at that.

[tsalo]

@eurunuela that would be awesome! Thanks!

[eurunuela]

Let me know what you think @tsalo.

[tsalo]

It looks good to me. Thanks @eurunuela!

[codecov]

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (d11f021) 89.00% compared to head (0fb81fe) 89.02%.
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #963      +/-   ##
==========================================
+ Coverage   89.00%   89.02%   +0.01%     
==========================================
  Files          27       27              
  Lines        3411     3417       +6     
  Branches      622      622              
==========================================
+ Hits         3036     3042       +6     
  Misses        227      227              
  Partials      148      148              

Files	Coverage Δ
tedana/io.py	87.50% <100.00%> (+0.12%)	⬆️
tedana/reporting/dynamic_figures.py	96.05% <ø> (ø)
tedana/reporting/html_report.py	92.66% <100.00%> (+0.06%)	⬆️
tedana/reporting/static_figures.py	96.34% <100.00%> (ø)
tedana/workflows/ica_reclassify.py	97.93% <100.00%> (+0.01%)	⬆️
tedana/workflows/tedana.py	81.88% <100.00%> (+0.06%)	⬆️

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

[handwerkerd]

In addition to the minor suggested change, the prefix isn't added to references.bib or the figures directory name.

I'd lean towards adding it for references.bib since each run of the program will overwrite the file and that stylistically not great. The contents should always be identical so I'm fine if you don't want to add it.

I'd lean against adding it to figures since all the files should be distinct in that directory, but figured I'd still flag it.

Otherwise LGTM

[tsalo]

I'd lean towards adding it for references.bib since each run of the program will overwrite the file and that stylistically not great. The contents should always be identical so I'm fine if you don't want to add it.

You're right, adding the prefix is a good idea. Most of the time, folks will run tedana with the same tree on each run. But there is the possibility that some people will want to run tedana with different settings on the same run, in which case the references file would contain different entries.

I'd lean against adding it to figures since all the files should be distinct in that directory, but figured I'd still flag it.

That was my thinking as well. As long as the figure files are unique, it should be fine.

[handwerkerd]

After approving, I noticed one minor thing that may or may not be worth changing. The four-echo integration test uses a pre-defined ICA mixing matrix:
https://github.com/tsalo/tedana/blob/1b97294eaed4e7111aba6a807c4ece38472a7270/tedana/tests/test_integration.py#L304

That file desc-ICA_mixing_static.tsv is copied into the output directory without any name change or prefix addition and then the same content is also appropriately named in sub-01_desc_ICA_mixing.tsv

I don't see this as a major issue for this PR. The one problem I could see it happening is if someone is using another package to run ICA and sending those matricies to tedana. In that case, it would be their responsibility to appropriately name their mixing matrix files.

That said, the supplied mixing matrix is copied without any checks on whether it's overwriting anything:
https://github.com/tsalo/tedana/blob/1b97294eaed4e7111aba6a807c4ece38472a7270/tedana/workflows/tedana.py#L519-L525
This should definitely include a check for io_generator.overwrite That could be added here or in a stand-alone PR.

[tsalo]

@handwerkerd if it's alright, I think I'd like to leave that for a separate PR. I'd like to see this one merged in and I don't have much time to work on it further at the moment.

[eurunuela]

LGTM! Thank you Taylor!

[handwerkerd]

@tsalo It looks like the conflicts should be easy to resolve then merge. You want to do that?

[handwerkerd]

Noticed a few things that must have been removed with the merge that were causing tests to fail. With the changed I'm including, all the tests were passing for me.

[tsalo]

@handwerkerd sorry about screwing up the merge. I thought the conflicts were spurious, since I was under the (mistaken) impression that the only PR that had been merged since I last updated was your documentation one.

[handwerkerd]

Now I see what happened. #747 added the new system info fields that were causing the conflict & that was merged just last week. Since that does interact with his PR, maybe it's worth waiting for @eurunuela to check nothing got messed up with the system info, but the output looks fine to me. Since he already approved, if Eneko doesn't have time for one more check during the next few days, I'm fine with you merging.

[eurunuela]

It looks like the conflicts were solved. Like @handwerkerd said, the outputs look fine.

[tsalo]

Since I have two approvals, I'm going to merge.