[ENH] BIDS Derivatives-compatible outputs #574

tsalo · 2020-05-22T16:08:56Z

Closes #146 and closes #649.

Changes proposed in this pull request:

Rename outputs to be as BIDS Derivatives-compatible as possible.
Reorganize metric and decomposition files. Metrics are now stored in TSVs, with associated JSONs describing the metrics, and decomposition files should match BEP012 convention.

TODO

T1gs.nii.gz (from gscontrol_raw())
desc-[PCA|ICA]R2ModelPredictions_X.nii.gz
desc-[PCA|ICA]S0ModelPredictions_X.nii.gz
desc-[PCA|ICA]AveragingWeights_X.nii.gz
Outputs of tedana page
Tedana workflow page
desc-ICA_metrics.tsv vs. desc-[tedana|AROMA]_metrics.tsv

EDIT: ~~Whoops, I think desc-tedana_metrics.tsv isn't specific enough since we'll have metrics from ICA and PCA.... 🤔~~ Circumvented by just having desc-PCA for the TEDPCA metrics and desc-tedana for the final TEDICA metrics.

tsalo · 2020-05-22T16:09:46Z

tedana/workflows/tedana.py

+    io.filewrite(
+        data_oc,
+        op.join(out_dir, 'desc-optcom_bold.nii.gz'),
+        ref_img
+    )


Write OC data earlier than before. Much more transparent this way.

tedana/metrics/kundu_fit.py

tsalo · 2020-05-22T16:11:17Z

tedana/io.py

-    fout = filewrite(ts, op.join(out_dir, 'ts_OC'), ref_img)
-    LGR.info('Writing optimally combined time series: {}'.format(op.abspath(fout)))
-
-    write_split_ts(ts, mmix, mask, comptable, ref_img, out_dir=out_dir, suffix='OC')


Just moved down. Not removed.

tsalo · 2020-05-22T16:11:25Z

tedana/io.py

    """
    acc = comptable[comptable.classification == 'accepted'].index.values
-
-    fout = filewrite(ts, op.join(out_dir, 'ts_OC'), ref_img)


Moved to main workflow.

tedana/gscontrol.py

tedana/decomposition/pca.py

codecov · 2020-05-22T20:46:47Z

Codecov Report

Merging #574 (cc9296b) into main (3897363) will decrease coverage by 0.06%.
The diff coverage is 93.57%.

@@            Coverage Diff             @@
##             main     #574      +/-   ##
==========================================
- Coverage   93.64%   93.58%   -0.07%     
==========================================
  Files          26       26              
  Lines        2029     2041      +12     
==========================================
+ Hits         1900     1910      +10     
- Misses        129      131       +2

Impacted Files	Coverage Δ
tedana/workflows/tedana.py	`89.66% <89.58%> (-0.16%)`	⬇️
tedana/metrics/kundu_fit.py	`95.87% <92.85%> (+0.29%)`	⬆️
tedana/selection/tedica.py	`93.37% <92.85%> (+0.37%)`	⬆️
tedana/decomposition/pca.py	`87.23% <94.11%> (+0.87%)`	⬆️
tedana/gscontrol.py	`100.00% <100.00%> (ø)`
tedana/io.py	`97.08% <100.00%> (-0.76%)`	⬇️
tedana/reporting/dynamic_figures.py	`100.00% <100.00%> (ø)`
tedana/reporting/html_report.py	`100.00% <100.00%> (ø)`
tedana/selection/tedpca.py	`83.33% <100.00%> (+0.52%)`	⬆️
tedana/workflows/t2smap.py	`93.90% <100.00%> (+0.31%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3897363...cc9296b. Read the comment docs.

tsalo · 2020-08-06T17:54:06Z

For transparency, here is what the ICA decomposition json will look like, with two example components:

{
    "Method": "Independent components analysis with FastICA algorithm implemented by sklearn. Components are sorted by Kappa in descending order. Component signs are flipped to best match the data.",
    "ica_00": {
        "Description": "ICA fit to dimensionally-reduced optimally combined data.",
        "classification": "rejected",
        "countnoise": 1119,
        "countsigFR2": 6128,
        "countsigFS0": 3778,
        "d_table_score": 18.4,
        "d_table_score_scrub": "n/a",
        "dice_FR2": 0.5568785491848324,
        "dice_FS0": 0.243486809765689,
        "kappa": 35.24615192099909,
        "kappa ratio": 2.408554759865709,
        "normalized variance explained": 0.017924586667012896,
        "original_classification": "rejected",
        "original_rationale": "I005",
        "rationale": "I001",
        "rho": 18.026996948546344,
        "signal-noise_p": 0.28063083660822047,
        "signal-noise_t": -1.0960543047701905,
        "variance explained": 9.670718133027448
    },
    "ica_01": {
        "Description": "ICA fit to dimensionally-reduced optimally combined data.",
        "classification": "accepted",
        "countnoise": 1277,
        "countsigFR2": 4097,
        "countsigFS0": 3496,
        "d_table_score": 14.8,
        "d_table_score_scrub": 11.8,
        "dice_FR2": 0.5252645699806231,
        "dice_FS0": 0.3942153186930905,
        "kappa": 33.957022187827526,
        "kappa ratio": 0.13136512366806724,
        "normalized variance explained": 0.0009146448952444163,
        "original_classification": "accepted",
        "original_rationale": "",
        "rationale": "",
        "rho": 33.23361928836013,
        "signal-noise_p": 0.7822498063437031,
        "signal-noise_t": 0.2769873372743333,
        "variance explained": 0.508159640579781
    },
}

Also rename the T1-like effect map.

tedana/gscontrol.py

tedana/tests/data/cornell_three_echo_outputs.txt

tsalo · 2020-10-22T16:52:33Z

Do we want to output sidecar jsons for these files as well? To specify Sources and Units at least?

emdupre · 2020-10-26T01:29:32Z

Do we want to output sidecar jsons for these files as well? To specify Sources and Units at least?

Sorry, which files ?

tsalo · 2020-10-26T01:36:28Z

We'd probably want jsons for all of the files. At least the core niftis.

EDIT: And a top-level derivatives description json?

tsalo · 2021-02-10T17:38:05Z

I think the only things left are:

~~Decide between desc-ICA and desc-tedana for the metrics files. We want to support multiple classification algorithms on the same decomposition.~~ Going with desc-tedana.
~~Add standard derivatives files, like dataset_description.json.~~ Done!

handwerkerd · 2021-02-18T22:09:27Z

I was just talking with @jbteves about this and I have one non-trivial concern. Sorry for not bringing this up earlier. This is going to be a major breaking change on outputs. Right now it seems to be close, but not quite matching the BIDS derivatives format and BIDS deviates isn't totally fixed either. That means we'll have another breaking change for output names in the near future (probably within the coming year).

You've put so much work into this that I don't want it sitting stagnate until we agree on perfect file naming. One middle ground might be to add a --BIDSoutput flag that would use the new names when true & the current names when false. The default would currently be false. Doing that would allow for incremental changes in BIDS derivatives output names within main for the people using those outputs (and this would allow for issue and fixes that come from active use) while not breaking things for people using the current output names. Knowing that file naming happens all over the code, this will be a bit of a headache & this whole topic is worth discussion (probably at tomorrows dev call). I figured I'd comment now instead of just speaking up during the dev call so that others could think about this in advance.

I also have a tangential suggestion. If we are making breaking changes to file names, is it time to finally get ride of 'Optimally Combined' / 'OptCom' / 'OC' in file names? There are several weighted summation methods in the code and @jsheunis has just proposed a new one. Having several methods all being saved as 'optimally combined' creates unnecessary confusion. Josh also noted 'Com' can mean 'components' in other situations, which adds to the confusion. Perhaps 'Weighted Sum' / 'WSum' / 'WS' is another option???

emdupre · 2021-02-18T22:41:40Z

This is going to be a major breaking change on outputs. Right now it seems to be close, but not quite matching the BIDS derivatives format and BIDS deviates isn't totally fixed either. That means we'll have another breaking change for output names in the near future (probably within the coming year).

I think we're well-timed for a breaking change as we approach our first minor-version release, which usually signals backwards-incompatible API changes :) I'd suggest that we pin ourselves to a set BIDS version for a while, and bump at the next minor-version release. But I agree with your broader point that this is worth discussing !

If we are making breaking changes to file names, is it time to finally get ride of 'Optimally Combined' / 'OptCom' / 'OC' in file names? There are several weighted summation methods in the code and @jsheunis has just proposed a new one. Having several methods all being saved as 'optimally combined' creates unnecessary confusion.

I'm fine with that change, though it's a good push for us to think about where we can explicitly denote the combination method. I'd argue that this is something that should be more transparently documented to end-users as they can call more methods.

tsalo · 2021-02-19T16:20:37Z

Per today's call, @handwerkerd and @jbteves will look into a name-swapping option next week.

jbteves · 2021-02-19T16:30:07Z

Quick thing: I noticed that this actually isn't quite compliant because we don't keep the subject and session for the outputs. Should we maybe also add a --prefix option so that this could be specified?

tsalo · 2021-02-19T16:36:24Z

Quick thing: I noticed that this actually isn't quite compliant because we don't keep the subject and session for the outputs. Should we maybe also add a --prefix option so that this could be specified?

This won't make tedana a full-on BIDS App. Instead, the goal is to make the suffixes and formats BIDS-compliant. That way, a BIDS App that uses tedana can take the subject, session, task, run, etc. entities and add them to the filenames we produce.

eurunuela · 2021-02-19T18:10:19Z

Just so we don't forget it, we discussed that updating the file names can be a burden if BIDS guidelines keep changing.

A solution could be to have a json file with the directory tree of tedana outputs. This would allow us to update the json file and forget about where in the code these files are being written.

docs/outputs.rst

tedana/decomposition/pca.py

jbteves · 2021-02-23T23:15:54Z

Alright, I have a significant draft of what's basically a BIDS derivatives enabler + major modularization of io.filewrite. I can either open a PR to this branch, which is merged in to my branch, or open a separate one to the repository where we can discuss the changes there. What's your preference @tsalo ?

tsalo · 2021-02-24T18:28:37Z

@jbteves I worry about keeping eyes on a PR to my fork. In my experience PRs like that tend to get ignored.. What if we change the target of this PR to a new branch and then merged? Then you could open your PR to that branch and we could have a draft PR from that branch to main simultaneously?

jbteves · 2021-02-24T21:41:45Z

I'm not quite sure I follow. In my branch, everything in this one is merged already.

tsalo · 2021-02-24T21:50:19Z

I can either open a PR to this branch

This would mean opening a PR to tsalo/tedana. My concern is that PRs to individuals' branches are often overlooked.

or open a separate one to the repository where we can discuss the changes there

This should work fine, but it won't let us evaluate your changes in isolation. They'll be mixed in with my changes.

My alternative was to (1) change the target of this PR to a new branch (e.g., bids-derivs), (2) merge this PR as-is into bids-derivs, (3) open your PR to bids-derivs, so that the diff will focus on your proposed changes, and (4) open a draft PR from bids-derivs to main, which we would merge once your PR to the new branch is merged.

jbteves · 2021-02-24T21:53:08Z

My alternative was to (1) change the target of this PR to a new branch (e.g., bids-derivs), (2) merge this PR as-is into bids-derivs, (3) open your PR to bids-derivs, so that the diff will focus on your proposed changes, and (4) open a draft PR from bids-derivs to main, which we would merge once your PR to the new branch is merged.

Oh, okay, I follow now. Sorry, I don't know how I misunderstood that. Yes, let's retarget to a branch on this repo that I can open a PR to it and shows up here for everyone to see. That sounds good, thanks!

tsalo · 2021-02-24T22:00:12Z

Done!

EDIT: You can target bids-derivatives with your PR.

BIDS Derivatives-compatible outputs.

21e479c

tsalo commented May 22, 2020

View reviewed changes

tedana/metrics/kundu_fit.py Outdated Show resolved Hide resolved

tsalo commented May 22, 2020

View reviewed changes

tedana/gscontrol.py Outdated Show resolved Hide resolved

tsalo commented May 22, 2020

View reviewed changes

tedana/decomposition/pca.py Outdated Show resolved Hide resolved

Fix outputs.

9dd2728

tsalo requested a review from emdupre May 22, 2020 21:08

tsalo added 2 commits July 12, 2020 22:56

Merge branch 'master' into ref/filenames-and-variables

41c9512

Fix filenames.

66b0a0c

tsalo added 4 commits September 22, 2020 13:15

Merge branch 'master' into ref/filenames-and-variables

000997e

Merge branch 'master' into ref/filenames-and-variables

eb23f74

Reformat MIR function with black.

737b19c

Also rename the T1-like effect map.

Update fiu_four_echo_outputs.txt

0b82189

tsalo commented Oct 22, 2020

View reviewed changes

tedana/gscontrol.py Outdated Show resolved Hide resolved

tsalo commented Oct 22, 2020

View reviewed changes

tedana/tests/data/cornell_three_echo_outputs.txt Outdated Show resolved Hide resolved

tsalo mentioned this pull request Nov 10, 2020

Produce BIDS Derivatives-compatible outputs ME-ICA/aroma#5

Closed

stale bot added the stale label Jan 24, 2021

Base automatically changed from master to main February 1, 2021 23:57

stale bot removed the stale label Feb 1, 2021

ME-ICA deleted a comment from stale bot Feb 6, 2021

tsalo added 2 commits February 6, 2021 14:09

Merge branch 'main' into ref/filenames-and-variables

d00b03a

Reorganize metric outputs.

51c7a7b

tsalo added 8 commits February 10, 2021 16:15

Add dataset_description.json.

6b4ab31

Update outputs and workflow description.

ed45069

Fix output list.

f7a4103

Fix more outputs.

fb7e8f8

Remove unused functions.

39a18f7

Fix imports.

245c7bd

Change desc-ICA to desc-tedana.

ed85699

Update docs.

cd1ce51

tsalo requested review from jbteves and handwerkerd February 12, 2021 22:00

tsalo mentioned this pull request Feb 13, 2021

[ENH] Generate BIDS Derivatives-compatible outputs ME-ICA/aroma#46

Merged

4 tasks

jbteves reviewed Feb 23, 2021

View reviewed changes

docs/outputs.rst Outdated Show resolved Hide resolved

jbteves reviewed Feb 23, 2021

View reviewed changes

tedana/decomposition/pca.py Show resolved Hide resolved

Update docs/outputs.rst

cc9296b

tsalo changed the base branch from main to bids-derivatives February 24, 2021 21:59

tsalo merged commit 0789f81 into ME-ICA:bids-derivatives Feb 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ENH] BIDS Derivatives-compatible outputs #574

[ENH] BIDS Derivatives-compatible outputs #574

tsalo commented May 22, 2020 •

edited

Loading

tsalo May 22, 2020

tsalo May 22, 2020

tsalo May 22, 2020

codecov bot commented May 22, 2020 •

edited

Loading

tsalo commented Aug 6, 2020

tsalo commented Oct 22, 2020

emdupre commented Oct 26, 2020

tsalo commented Oct 26, 2020 •

edited

Loading

tsalo commented Feb 10, 2021 •

edited

Loading

handwerkerd commented Feb 18, 2021 •

edited

Loading

emdupre commented Feb 18, 2021

tsalo commented Feb 19, 2021

jbteves commented Feb 19, 2021

tsalo commented Feb 19, 2021

eurunuela commented Feb 19, 2021

jbteves commented Feb 23, 2021 •

edited

Loading

tsalo commented Feb 24, 2021

jbteves commented Feb 24, 2021

tsalo commented Feb 24, 2021

jbteves commented Feb 24, 2021

tsalo commented Feb 24, 2021 •

edited

Loading

[ENH] BIDS Derivatives-compatible outputs #574

[ENH] BIDS Derivatives-compatible outputs #574

Conversation

tsalo commented May 22, 2020 • edited Loading

TODO

tsalo May 22, 2020

Choose a reason for hiding this comment

tsalo May 22, 2020

Choose a reason for hiding this comment

tsalo May 22, 2020

Choose a reason for hiding this comment

codecov bot commented May 22, 2020 • edited Loading

Codecov Report

tsalo commented Aug 6, 2020

tsalo commented Oct 22, 2020

emdupre commented Oct 26, 2020

tsalo commented Oct 26, 2020 • edited Loading

tsalo commented Feb 10, 2021 • edited Loading

handwerkerd commented Feb 18, 2021 • edited Loading

emdupre commented Feb 18, 2021

tsalo commented Feb 19, 2021

jbteves commented Feb 19, 2021

tsalo commented Feb 19, 2021

eurunuela commented Feb 19, 2021

jbteves commented Feb 23, 2021 • edited Loading

tsalo commented Feb 24, 2021

jbteves commented Feb 24, 2021

tsalo commented Feb 24, 2021

jbteves commented Feb 24, 2021

tsalo commented Feb 24, 2021 • edited Loading

tsalo commented May 22, 2020 •

edited

Loading

codecov bot commented May 22, 2020 •

edited

Loading

tsalo commented Oct 26, 2020 •

edited

Loading

tsalo commented Feb 10, 2021 •

edited

Loading

handwerkerd commented Feb 18, 2021 •

edited

Loading

jbteves commented Feb 23, 2021 •

edited

Loading

tsalo commented Feb 24, 2021 •

edited

Loading