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

Set the field 'TaskName' in the json file for multiecho func data #420

Merged
merged 3 commits into from
Mar 27, 2020

Conversation

pvelasco
Copy link
Contributor

Fixes #417

@codecov
Copy link

codecov bot commented Feb 20, 2020

Codecov Report

Merging #420 into master will increase coverage by 0.07%.
The diff coverage is 92.3%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #420      +/-   ##
==========================================
+ Coverage   74.71%   74.79%   +0.07%     
==========================================
  Files          35       35              
  Lines        2757     2773      +16     
==========================================
+ Hits         2060     2074      +14     
- Misses        697      699       +2
Impacted Files Coverage Δ
heudiconv/convert.py 80% <92.3%> (+0.59%) ⬆️
heudiconv/heuristics/reproin.py 81.96% <0%> (-0.16%) ⬇️

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 756fd80...7a5f13e. Read the comment docs.

heudiconv/convert.py Outdated Show resolved Hide resolved
Copy link
Member

@yarikoptic yarikoptic left a comment

Choose a reason for hiding this comment

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

Thank you for pushing better support for multiecho forward!
I think for this PR we need

  • take the code which embeds task label out from embed_nifti, and move into a function like you did
    • add a dedicated unit test for it
  • remove min_meta option from that embed_nifti and just do not bother even using it in embed_metadata_from_dicoms if min_meta is set
  • call that new function in case of bids for all those sidecar files (regardless of their number), thus retaining that warning as I have mentioned

heudiconv/convert.py Outdated Show resolved Hide resolved
heudiconv/convert.py Show resolved Hide resolved
@pvelasco
Copy link
Contributor Author

  • take the code which embeds task label out from embed_nifti, and move into a function like you did

Do you want this function in convert.py, dicoms.py or utils.py?

@yarikoptic
Copy link
Member

  • take the code which embeds task label out from embed_nifti, and move into a function like you did

Do you want this function in convert.py, dicoms.py or utils.py?

has nothing to do with DICOMs per se, will be used in convert, so let's stick it there for now

Addresses initial comments in PR nipy#470
It handles now either a path or a list of paths of json files.
The setting of the "TaskName" is removed from "embed_metadata_from_dicoms".
Addresses some change requests in PR nipy#470
@pvelasco
Copy link
Contributor Author

  • remove min_meta option from that embed_nifti and just do not bother even using it in embed_metadata_from_dicoms if min_meta is set

Do we need to call embed_metadata_from_dicoms at all if min_meta is set?
I mean, in convert.py, I can call add_taskname_to_infofile, and then skip the if statements related embed_metadata_from_dicoms if min_meta is set...

@yarikoptic
Copy link
Member

Do we need to call embed_metadata_from_dicoms at all if min_meta is set?
I mean, in convert.py, I can call add_taskname_to_infofile, and then skip the if statements related embed_metadata_from_dicoms if min_meta is set...

good observation. That embed_nifti still has the same code as now in add_taskname_to_infofile, and that is (as far as I see it) the only piece which is used if min_meta, so I will strip it away from there (after all it is not embedding any metadata from dicoms ;)), and then indeed we could avoid calling embed_metadata_from_dicoms if minmeta. That with_prov handling wouldn't be run, but if we are not embedding (min_meta is set) I think it is ok. I will push this RFing shortly after making piece of mind (and tests) with it ;)

@yarikoptic
Copy link
Member

actually -- I will just merge this one, and then follow up with a new PR on top

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.

'TaskName' not set in json files for multi-echo data
2 participants