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

[ENH] start bidsifying output #80

Merged
merged 28 commits into from
Mar 25, 2024
Merged

[ENH] start bidsifying output #80

merged 28 commits into from
Mar 25, 2024

Conversation

Remi-Gau
Copy link
Collaborator

@Remi-Gau Remi-Gau commented Dec 14, 2023

closes #32

  • add dataset description
  • add basic metadata to output
  • files are saved in sub-<sub_id>/func folder
  • add a meas entity to filename
  • bids compliant output:
    • one output file per input file
    • one file matrix and one for time series
  • update output description in doc

giga_connectome/utils.py Outdated Show resolved Hide resolved
giga_connectome/utils.py Outdated Show resolved Hide resolved
giga_connectome/utils.py Outdated Show resolved Hide resolved
@Remi-Gau Remi-Gau marked this pull request as ready for review December 14, 2023 19:24
@codecov-commenter
Copy link

codecov-commenter commented Dec 14, 2023

Codecov Report

Attention: Patch coverage is 96.22642% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 90.28%. Comparing base (b88333f) to head (784d9b5).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #80      +/-   ##
==========================================
+ Coverage   89.80%   90.28%   +0.48%     
==========================================
  Files          11       11              
  Lines         500      525      +25     
==========================================
+ Hits          449      474      +25     
  Misses         51       51              
Files Coverage Δ
giga_connectome/connectome.py 94.87% <ø> (ø)
giga_connectome/mask.py 93.93% <100.00%> (-0.07%) ⬇️
giga_connectome/methods.py 100.00% <ø> (ø)
giga_connectome/postprocess.py 89.55% <100.00%> (+2.98%) ⬆️
giga_connectome/workflow.py 88.37% <100.00%> (-0.27%) ⬇️
giga_connectome/utils.py 88.67% <92.59%> (+1.33%) ⬆️

@htwangtw
Copy link
Collaborator

There's a big problem with this structure - our file system handles small files poorly, hence I came up with the solution of dumping everything into one h5 file for group level.

@Remi-Gau
Copy link
Collaborator Author

There's a big problem with this structure - our file system handles small files poorly, hence I came up with the solution of dumping everything into one h5 file for group level.

at the group level? or the participant level?

in any case, how about we add an output_format flag to the CLI to allow BIDS output, otherwise it keeps the previous format?

@htwangtw
Copy link
Collaborator

There's a big problem with this structure - our file system handles small files poorly, hence I came up with the solution of dumping everything into one h5 file for group level.

at the group level? or the participant level?

An illicit use of using group level to generate participant level results but bundle them in one h5 file.
I really want to make it more BIDS compliant. One midway solution I was thinking of is to have the two json files + everything else in a h5 file?

in any case, how about we add an output_format flag to the CLI to allow BIDS output, otherwise it keeps the previous format?

Sounds good.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit extra to this PR but it felt like a minimum description on how to run a demo was necessary


The output file name is: `group/atlas-<atlas_name>_meas-PearsonCorrelation_desc-<denoising_strategy>.h5`

## Examples
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added examples of the output generated by the demo

@Remi-Gau
Copy link
Collaborator Author

Note that this probably should trigger a minor version bump because the output data structure will be defo incompatible with what previous versions of the app were doing.

giga_connectome/utils.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@htwangtw htwangtw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. For the BIDS compliant output, I am inclined to just use tsv as the output format.

  2. For the non-BIDS compliant h5 output, I want to make everything that's not the json file into one h5 file, so essentially revert to what it was like before but just change the dataset names inside the h5 file. Does this make sense? Sorry for the trouble

@htwangtw
Copy link
Collaborator

htwangtw commented Feb 1, 2024

@Remi-Gau i had a chat with Pierre and reached the conclusion. we will follow bids through and through, and save things in tsv, so this will make things a lot easier.

@htwangtw htwangtw mentioned this pull request Mar 25, 2024
5 tasks
Comment on lines 181 to 188
# dump timeseries to h5
h5_filename = connectome_path / utils.output_filename(
source_file=Path(img.filename).stem,
atlas=atlas["name"],
extension="h5",
strategy=strategy["name"],
desc=desc,
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to be fully BIDS compliant, shall we save time series to h5 as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah am working on this

ideally maybe even in nifti, no?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no nifty since we are not outputting seed based maps?
The only sensible nifti we can output from here is the denoised and smoothed nifti, but that would be out of scope IMO

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok will dump that in tsv then

Comment on lines -92 to -96
if analysis_level != "participant":
raise Warning(
"Only participant level analysis is supported. Other levels "
"would be ignored, and will run the participant level."
)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

realizing I just completely removed this from the code

but I feel that given that this is group level is not allowed by the CLI this warning will never be hit in practice

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah that makes sense

.pre-commit-config.yaml Outdated Show resolved Hide resolved
@htwangtw htwangtw merged commit e303c9f into bids-apps:main Mar 25, 2024
15 checks passed
@Remi-Gau Remi-Gau deleted the folder branch March 25, 2024 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BIDS-connectome compliant output layout
3 participants