[ENH] Remove accepted and rejected outputs #822

tsalo · 2021-10-22T18:23:44Z

Closes #819.

Changes proposed in this pull request:

Drop accepted and rejected component-related outputs.
Remove unused n_vols parameter from io.writeresults.
Remove "optcom" from certain output names.
Update docs/outputs.rst.
Update integration test outputs.

Removed derivatives

desc-optcomRejected_bold.nii.gz
desc-optcomAccepted_bold.nii.gz
desc-ICAAccepted_components.nii.gz
desc-ICAAccepted_stat-z_components.nii.gz
echo-[echo]_desc-Accepted_bold.nii.gz
echo-[echo]_desc-Rejected_bold.nii.gz
desc-optcomAcceptedMIRDenoised_bold.nii.gz
desc-ICAAcceptedMIRDenoised_components.nii.gz

Renamed derivatives

desc-optcomMIRDenoised_bold.nii.gz —> desc-MIRDenoised_bold.nii.gz
echo-{echo}_desc-Denoised_bold.nii.gz —> echo-{echo}_desc-denoised_bold.nii.gz
desc-optcomDenoised_bold.nii.gz —> desc-denoised_bold.nii.gz

To do:

Document prefix a bit more, so it's clear that the prefix isn't automatically inferred. From Update output names to match BIDS derivatives RC2 conventions #146 (comment).

Also remove the term "optcom" from certain outputs.

tsalo · 2021-10-25T15:12:50Z

I still need to change which maps are required for the reports, but are we happy with the output changes overall?

handwerkerd

This looks good overall.

docs/outputs.rst

handwerkerd · 2021-11-15T20:02:18Z

tedana/resources/config/outputs.json

@@ -27,25 +27,9 @@
        "orig": "betas_OC",
        "bidsv1.5.0": "desc-ICA_stat-z_components"
    },
-    "ICA accepted components img": {
-        "orig": "betas_hik_OC",
-        "bidsv1.5.0": "desc-ICAAccepted_components"


Is this the first place we should add a new file name version code? That is we are changing file names and preventing people from saving files with these older names. We can rename the current ones something like "bidsv1.5.0old". While I don't think anyone would want to use these older names, the big benefit is, if someone has files with these old names, they'd/we'd be able to quickly remember what they now correspond to.

BIDS is on v1.6.0 right now, so I'd lean toward adding that and changing the default over renaming v1.5.0. WDYT?

I now realize that we should have done something like this for #736 as well... and we should probably have our different versions reflected in the outputs tables too!

Aligning with v1.6.0 with this PR sounds sounds like a great solution.

Did #736 change any file names or just change what was in some of the files? If it changed a file name, I could see back-adding that into the output filename table, but this seems really minor. I'd say don't bother or consider it a really low priority.

It changed the T2* and S0 maps. T2starmap.nii.gz --> desc-limited_T2starmap.nii.gz and desc-full_T2starmap.nii.gz --> T2starmap.nii.gz.

One caveat: I don't believe any of our changes are driven by changes to the specification. Our 1.5.0 names should be completely compatible with 1.6.0, and vice versa. It's just a useful way to version our own names without adding in entirely new elements to the naming convention. Well, it's useful as long as we don't make major changes to our naming convention faster than BIDS releases, I guess.

Beyond adding the v1.6.0 naming convention (and reverting name changes, but not removals, from v1.5.0) in 280771c, here's what I'm thinking could help:

Unfortunately, this would involve working through our commit history to pin down when each filename was changed and which tedana versions the changes apply to, which I don't want to tackle in this PR, but I think the overall idea is solid.

EDIT: Or I guess meica.py - v0.0.10 instead of Original filename

I suggest just opening a new issue for adding in the intermediate file naming convention.

FWIW, when we made this file naming decision, I think we talked about creating new file naming options that were linked to a released version. That is, we'd always be able to say "Save files as they were in tedana v0.0.9" I don't see that in the current code, but this discussion shows why such an option might be useful. Whatever we call each naming convention, there would also be an ordered list saying in which version each convention was the default. This wouldn't be too hard to add, but it would definitely be a new issue.

@handwerkerd that is correct, I proposed that if this becomes an issue we could alias a few tedana versions to certain naming conventions (not track them in the JSON file itself).

The logical place to do this would be in the input parser in the init of io.OutputGenerator The one quirk is that would mean every time we edit outputs.json, we'd also want to add to the parser to peg the current version with the previous file names.

codecov · 2021-11-19T19:03:19Z

Codecov Report

Patch coverage: 100.00% and project coverage change: +4.28 🎉

Comparison is base (09929c2) 88.91% compared to head (8e07e4d) 93.20%.

❗ Current head 8e07e4d differs from pull request most recent head d57a728. Consider uploading reports for the commit d57a728 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #822      +/-   ##
==========================================
+ Coverage   88.91%   93.20%   +4.28%     
==========================================
  Files          27       27              
  Lines        3375     2193    -1182     
  Branches      618        0     -618     
==========================================
- Hits         3001     2044     -957     
+ Misses        227      149      -78     
+ Partials      147        0     -147

Impacted Files	Coverage Δ
tedana/decomposition/pca.py	`86.51% <ø> (+11.12%)`	⬆️
tedana/gscontrol.py	`100.00% <ø> (ø)`
tedana/workflows/tedana.py	`89.85% <ø> (+8.90%)`	⬆️
tedana/io.py	`93.37% <100.00%> (+5.99%)`	⬆️
tedana/reporting/static_figures.py	`98.55% <100.00%> (+2.20%)`	⬆️

... and 29 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

tsalo · 2023-08-24T17:46:34Z

I believe we decided recently that it would be easier to just start over from scratch on this instead of trying to fix up the PR.

tsalo added 8 commits October 22, 2021 14:17

Drop accepted and rejected outputs.

3c942bb

Also remove the term "optcom" from certain outputs.

Stop generating accepted and rejected outputs.

70daca6

Update integration test outputs.

b56c836

Remove unused parameter.

b980089

Update outputs documentation.

ba9cda0

Update gscontrol.py

9097e9a

Update gscontrol.py

3c5cb38

Fix failing test.

c7d8bc0

handwerkerd reviewed Nov 15, 2021

View reviewed changes

tsalo added 10 commits November 16, 2021 11:19

Add links to gscontrol docs.

1b1e34a

Add warning about GSR to docstring.

c6f09d3

Try documenting old version in table.

996b16c

Add bidsv1.6.0 convention.

280771c

Improve table.

9ad7143

Update outputs.rst

a90dcbc

Get MIR carpets working again.

3d14f7b

Add text about outputs tables.

aa705de

Fix formatting issues.

d2c2643

No index_col.

8e07e4d

tsalo marked this pull request as ready for review November 19, 2021 18:24

tsalo mentioned this pull request Nov 19, 2021

Document filename changes and conventions in outputs tables #829

Open

tsalo requested review from handwerkerd and jbteves November 19, 2021 19:28

tsalo mentioned this pull request Nov 30, 2021

Update output names to match BIDS derivatives RC2 conventions #146

Closed

tsalo added 4 commits December 2, 2021 12:58

Add note for ME-ICA#146 (comment).

1849a26

Merge remote-tracking branch 'upstream/main' into output-change

592eeee

Update.

6d6abce

Update io.py

d57a728

tsalo closed this Aug 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ENH] Remove accepted and rejected outputs #822

[ENH] Remove accepted and rejected outputs #822

tsalo commented Oct 22, 2021 •

edited

Loading

tsalo commented Oct 25, 2021

handwerkerd left a comment

handwerkerd Nov 15, 2021

tsalo Nov 16, 2021

tsalo Nov 16, 2021

handwerkerd Nov 16, 2021

tsalo Nov 16, 2021 •

edited

Loading

tsalo Nov 16, 2021 •

edited

Loading

handwerkerd Nov 16, 2021

jbteves Nov 17, 2021

handwerkerd Nov 17, 2021

codecov bot commented Nov 19, 2021 •

edited

Loading

tsalo commented Aug 24, 2023

[ENH] Remove accepted and rejected outputs #822

[ENH] Remove accepted and rejected outputs #822

Conversation

tsalo commented Oct 22, 2021 • edited Loading

Removed derivatives

Renamed derivatives

tsalo commented Oct 25, 2021

handwerkerd left a comment

Choose a reason for hiding this comment

handwerkerd Nov 15, 2021

Choose a reason for hiding this comment

tsalo Nov 16, 2021

Choose a reason for hiding this comment

tsalo Nov 16, 2021

Choose a reason for hiding this comment

handwerkerd Nov 16, 2021

Choose a reason for hiding this comment

tsalo Nov 16, 2021 • edited Loading

Choose a reason for hiding this comment

tsalo Nov 16, 2021 • edited Loading

Choose a reason for hiding this comment

handwerkerd Nov 16, 2021

Choose a reason for hiding this comment

jbteves Nov 17, 2021

Choose a reason for hiding this comment

handwerkerd Nov 17, 2021

Choose a reason for hiding this comment

codecov bot commented Nov 19, 2021 • edited Loading

Codecov Report

tsalo commented Aug 24, 2023

tsalo commented Oct 22, 2021 •

edited

Loading

tsalo Nov 16, 2021 •

edited

Loading

tsalo Nov 16, 2021 •

edited

Loading

codecov bot commented Nov 19, 2021 •

edited

Loading