[ENH] Make outputs BIDS derivatives-compatible #152
Conversation
tsalo
added
the
enhancement
Codecov Report
@@ Coverage Diff @@
## master #152 +/- ##
==========================================
- Coverage 46.11% 45.35% -0.76%
==========================================
Files 33 33
Lines 2045 2154 +109
==========================================
+ Hits 943 977 +34
- Misses 1102 1177 +75
Continue to review full report at Codecov.
|
Now it can take in any number of keyword arguments to be added as fields in the filename.
Also rename generate_fname to gen_fname. # Conflicts: # tedana/tests/test_utils.py # tedana/workflows/tedana.py
I realized that not every derivative would be a “bold” file.
tsalo
removed
the
enhancement
tsalo
modified the milestones:
transparent and reproducible processing,
workflow integration: BIDS
# Conflicts: # tedana/selection/select_comps.py
# Conflicts: # docs/outputs.rst
# Conflicts: # tedana/workflows/tedana.py
# Conflicts: # tedana/workflows/tedana.py
Updates to gen_fname: - Remove duplicate keys (e.g., “desc”) when adding keys to filename. - Use basefile’s directory (without editing). - Add echo as first key in output filename if provided. Other updates: - Use outdir in basefile to eliminate need for out_dir argument in all file writing functions. - Update filenames to match proposed conventions. - Add bf argument to tedpca, gscontrol_mmix, write_split_ts, writefeats, writeresults, writeresults_echoes, and gscontrol_raw to call gen_fname within these functions. - Get automatic gzipping working in filewrite. - Delete unused ctabsel. Still need to update function docstrings with updated arguments and write out metadata (jsons). Also maybe some record of the arguments used.
tsalo
changed the title
|
Tests are now passing and figures are outputted in BIDS format. The names may need to be adjusted, but everything generally looks good to me. This is ready for review. |
# Conflicts: # tedana/decomposition/eigendecomp.py # tedana/io.py # tedana/model/fit.py # tedana/tests/test_gscontrol.py # tedana/workflows/tedana.py
docs/outputs.rst
Outdated
| ======================================== ===================================================== | ||
| Filename Content | ||
| ======================================== ===================================================== | ||
| *_desc-limited_t2s.nii.gz Limited estimated T2* 3D map. |
There was a problem hiding this comment.
Yes, please ! Let's update for T2Starmap and S0map.
docs/outputs.rst
Outdated
| of echoes, starting with 2. | ||
| *_desc-ascendingEstimates_s0.nii.gz Voxel-wise S0 estimates using ascending numbers | ||
| of echoes, starting with 2. | ||
| *_desc-full_t2s.nii.gz Full T2* map/time series. The difference between |
There was a problem hiding this comment.
I think we can just get rid of this desc. It should be clear that it's "full" in comparison to the desc-limited files above !
There was a problem hiding this comment.
I only don't like this because full is only used for optimal combination. It contains T2* estimates for voxels with only one good echo, which in my mind makes it less of a "full" map and more of a... liberal map? I'm not sure what to call it, but I think we should rename it. What about changing limited to nothing (dropping desc) and changing full to liberal?
docs/outputs.rst
Outdated
| good data, the full map uses the single echo's | ||
| value while the limited map has a NaN. Only used | ||
| for optimal combination. | ||
| *_desc-full_s0.nii.gz Full S0 map/time series. Only used for optimal |
There was a problem hiding this comment.
See previous comment on dropping this desc key-value pair !
| *_desc-T1gs_bold.nii.gz Spatial global signal | ||
| *_globalSignal_regressors.tsv Time series of global signal from optimally combined | ||
| data. | ||
| *_desc-optcomWithGlobalSignal_bold.nii.gz Optimally combined time series with global signal |
There was a problem hiding this comment.
Isn't this already covered on L22 ?
There was a problem hiding this comment.
The gsr method outputs two files that contain the same contents, I guess, although I think the duplicate is actually *_desc-optcomNoGlobalSignal_bold.nii.gz.
| ================================================== ===================================================== | ||
| Filename Content | ||
| ================================================== ===================================================== | ||
| *_desc-optcomAccepted_min.nii.gz T1-like effect |
There was a problem hiding this comment.
Should we comment on the derivs PR to suggest it ?
| @@ -185,6 +185,8 @@ def tedpca(catd, OCcatd, combmode, mask, t2s, t2sG, | |||
| Boolean mask array | |||
| ref_img : :obj:`str` or img_like | |||
| Reference image to dictate how outputs are saved to disk | |||
| bf : :obj:`str` | |||
There was a problem hiding this comment.
Can we rename this to something a little more intuitive ? You mean everything before like echo- in the original filename, right ?
There was a problem hiding this comment.
That part of the filename and the output directory as well. Do you have a preference for the new name? bf stands for base filename, so that's a possibility.
|
I think there are a few general discussion points that we might want to rope in others for, as they extend more broadly into BIDS. In particular, there are two immediate things I can think of that will make our file names much more intuitive.
|
|
@emdupre I've added the decomposition files now. Please let me know what you think of them from the integration test artifacts. |
Also fix up docstrings and eliminate step where component number is set as a column. Always keep it as the index of the DataFrame.
* Speed up spatclust function. This also clusters positive and negative values separately. * Rename spatclust to threshold_map and add binarize/sided arguments. * Replace manually generated binary structure with function-made one.
# Conflicts: # tedana/model/__init__.py # tedana/model/fit.py # tedana/selection/select_comps.py # tedana/workflows/tedana.py
# Conflicts: # tedana/decomposition/eigendecomp.py # tedana/io.py # tedana/model/fit.py # tedana/viz.py # tedana/workflows/tedana.py
|
@tsalo You may want to update this branch after the refactor. |
|
I'm going to close this. I'll probably circle back and take another crack at it at a later date. |
Closes #146, closes #180, and closes #252.
We should wait until #136 is merged to deal with this PR, as this will involve editing every function that generates files.Changes proposed in this pull request:
gen_fnameto generate BIDS derivatives-compatible filenames from a base file (input file corresponding to first echo), a description for the output file, the data type, and the file extension.labelwithout_dirin workflows.bf(base filename) argument to all file-generating functions.filewrite.ctabselfunction.