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: Adds populate_intended_for for fmaps #482

Merged
merged 94 commits into from
Feb 24, 2022

Conversation

pvelasco
Copy link
Contributor

This commits adds a new populate_intended_for functionality to heudiconv. As its name indicates, it populates the "IntendedFor" field in the fmap json files of a given session.
At the moment, it can be called as a --command option. In the future, it should also be called automatically every time there is a bids extraction.

  • Currently, if the "ShimSetting" field is present in the fmap json file, it must match the "ShimSetting" in the other run (func, dwi, etc.) for this other run to be added to the "IntededFor" for that fmap. If the "ShimSetting" is not present in the json files, the match is based on the <acq> entity in the fmap file name: jsons with acq-dwi will be intended for runs in the dwi folder, acq-fmri or acq-func will be intended for runs in the func folder, etc. The logic for enforcing that the "ShimSetting" (if present) matches is that if the scanner changes the shims between a fmap and its "IntededFor" run, the actual B0 field will be different between them, so the field measured (or estimated, from a pepolar pair) from the fmap should not be applied to correct the other run.
  • If more than one fmap fulfills the requirement above, the first of them (and any other fmap with matching <acq> and <run> entities, so that pepolar pairs and magnitude/phase pair have the same "IntededFor" runs) will get the "IntendedFor". In the future, we might want to change this (e.g., so that it uses the one closest in time), or we might want to give the user the option to choose one or the other.

It addresses #474.

It also adds the corresponding unit test
It adds a function to populate the "IntendedFor" field to the fmap jsons. It also adds the corresponding test.  Minor modification to `create_tree` for the case of a `name` ending in `.json`.
It also adds the corresponding test
Also, fixes minor bug in ignoring `_sbref` images.
...rather than underscores (`_`) for consistency with other commands

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Merges nipy's master
@pvelasco pvelasco force-pushed the adds_populate_intended_for branch from 2699959 to 34b2acc Compare December 18, 2020 20:59
@codecov
Copy link

codecov bot commented Dec 18, 2020

Codecov Report

Merging #482 (1c5ca68) into master (92eecfe) will increase coverage by 3.30%.
The diff coverage is 94.94%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #482      +/-   ##
==========================================
+ Coverage   77.89%   81.20%   +3.30%     
==========================================
  Files          41       41              
  Lines        3203     3782     +579     
==========================================
+ Hits         2495     3071     +576     
- Misses        708      711       +3     
Impacted Files Coverage Δ
heudiconv/cli/run.py 91.48% <ø> (ø)
heudiconv/convert.py 87.22% <77.77%> (-0.25%) ⬇️
heudiconv/main.py 78.66% <78.26%> (+2.10%) ⬆️
heudiconv/bids.py 89.68% <95.00%> (+5.06%) ⬆️
heudiconv/heuristics/example.py 5.55% <100.00%> (+1.33%) ⬆️
heudiconv/tests/test_bids.py 100.00% <100.00%> (ø)
heudiconv/tests/test_convert.py 100.00% <100.00%> (ø)
heudiconv/tests/test_main.py 100.00% <100.00%> (ø)
heudiconv/tests/test_utils.py 100.00% <100.00%> (ø)
heudiconv/utils.py 91.36% <100.00%> (+1.32%) ⬆️
... and 4 more

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 92eecfe...1c5ca68. Read the comment docs.

@yarikoptic
Copy link
Member

THANK YOU @pvelasco , I will review/try it out in upcoming days and provide feedback. Happy holidays!

…s expected IntendedFor

This way, when we write future test cases, the test function can be reused as long as it know what the expected IntendedFor are.
Removing an element from a (sorted) list while looping through the list was not a good idea, and it failed in some cases. Now we keep a parallel list with elements already accounted for and, while looping through the list, we only act if the element is not already accounted for.
It coves the cases in which the fmaps are in the form of magnitude/phase maps
To prepare for a future `test_convert.test_convert`, which requires mocking some functions
It also adds the corresponding unit tests.
We make sure we don't try to write to the root directory
@yarikoptic
Copy link
Member

I saw activity ongoing through the productive (for some) holidays thus did not review. Is it ready @pvelasco ? ;-)

@pvelasco
Copy link
Contributor Author

Yes. I think it is ready. I had some bugs in the unit tests, since I could not test them locally (#486), but they are now fixed.
If preferred, I could squash all those Bug Fix commits into one. Otherwise, it is ready for merging.

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.

I only started to review (so review is not complete, but decided to not delay until it is "well done" ;)), and still thinking about it

  • I really like an idea to (ab)use _acq- as an indicator for such grouping. I even at some point thought to establish a dedicated one within reproin heuristic, so the "loosely specified in BIDS" _acq- seems to be reasonable one to use. I just still wonder if such new behavior would not somehow introduce some undesired assignment, in particular if ShimSettings are not there (Philips, GE, ?).
  • we might want to add additional, or alternative (if no ShimSettings?) constraints, e.g. based on resolution and "sform" (have not looked what would be the analog at DICOM level, ImageOrientationPatientDICOM?). If I got it right it is still debatable if fieldmaps could be acquired/reconstructed at resolution different from the target images... but may be we for starters could constrain based on entire sform, thus at least guaranteeing that positioning and sizes are the same
  • in one of our use cases I noted that shimming was just a bit different:
[-7371, 5580, 11925, -269, -131, -448, -111, -459] vs
[-7372, 5589, 11937, -276, -130, -360, -157, -451]

yet to clarify with original collection site/techs -- but it raised the question on either it is completely useless, or we could still associate, and thus here may be allow for some %difference?

  • in case of ambiguity (multiple fmaps matching) - should we take the one closest in time?
  • since different people might want/have different strategies - should this be more of a heuristic, i.e. other heuristics could reuse in theirs etc... or at least may be we should use this one only if heuristic does not provide one

heudiconv/bids.py Outdated Show resolved Hide resolved
heudiconv/bids.py Outdated Show resolved Hide resolved
heudiconv/bids.py Outdated Show resolved Hide resolved
j for j in glob(op.join(path_to_bids_session, '*/*.json')) if not (
j in fmap_jsons
# j[:-5] removes the '.json' from the end
or j[:-5].endswith('_sbref')
Copy link
Member

Choose a reason for hiding this comment

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

just thinking out loud: we so need "BIDS filename" parser -- our code is full of similar constructs. #452

heudiconv/bids.py Outdated Show resolved Hide resolved
pvelasco and others added 2 commits February 18, 2021 15:42

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Co-authored-by: Yaroslav Halchenko <[email protected]>
@neurorepro
Copy link
Contributor

Hello @pvelasco and @yarikoptic , this is a great PR !

I had some issues when using it in my case. The main problems were:

  • the ImagingVolume parameter did not lead to a proper comparison between fmap and modality json info
  • the AcquisitionLabel did not allow for selecting custom acquisition label, but only modality. This does not work in my use case as i have several field maps for several different fmri tasks (i.e. all "func" modality)

I made a PR to address the first issue here, and i already have code to address the second issue but it'd be easier if the first PR could be validated before i submit the second PR.

What do you guys think ?

pvelasco and others added 2 commits February 11, 2022 13:53

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
BF: In `find_compatible_fmaps_for_run`, actually check if the first element of the parameters list is a string.
Thanks, @neurorepro
@neurorepro
Copy link
Contributor

@pvelasco and @yarikoptic I have now made this PR to address custom labels, which includes updated doc and test suite.

@neurorepro
Copy link
Contributor

@pvelasco and @yarikoptic , how can i help finalize this PR so that it can be merged into the main branch ?

update_json(fm_json, {"IntendedFor": intended_for}, pretty=True)


class BIDSFile(object):
Copy link
Member

Choose a reason for hiding this comment

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

woohoo -- here it is also! ideally #544 should be finished/merged first (so would be driven by a specific copy of the schema etc), but doesn't need to be. If this PR is merged first, that one should be rebased/code moved/RFed and would immediately take advantage of this use-case use

@yarikoptic
Copy link
Member

@pvelasco and @yarikoptic , how can i help finalize this PR so that it can be merged into the main branch ?

clearly I would not be able to evaluate it in full/detailed, but I will resolve conflicts now and may be do some minor tune ups, and push back... then whenever green -- we should just merge and go from there. meanwhile -- more testing never hurts!

…ded_for

* origin/master:
  Update test to match new behavior.
  Drop suffix parameter from update_complex_name().
  Remove unused imports.
  Enhance lists of entities in filename updaters.
  Use "base_fn" instead of "fn" in tests.
  Rename suffix to suffix_counter in update_complex_name.
  Work on fixing issues with multi-file converters.
  BF: Fix the order of the 'echo' entity in the filename
  try a simple fix for wrongly ordered files in tar file
  Use False instead of NaN bc filter wasn't catching NaNs.
  Convert variables to lists and check for lists in functions.

Conflicts:
	heudiconv/tests/test_convert.py -- just in imports
@yarikoptic
Copy link
Member

Thank you @pvelasco for all the work on this PR and thank you @neurorepro for the help! I did some testing locally -- seems to work nicely. Let's proceed!

@nikhil153
Copy link

nikhil153 commented Mar 30, 2022

Thanks for very useful addition @pvelasco, @neurorepro, and @yarikoptic. I am doing some local tests with the main branch, and seeing this warning when I run with "--command populate-intended-for" option.

INFO: Running heudiconv version 0.10.0 latest 0.10.0
INFO: Adding "IntendedFor" to the fieldmaps in /scratch/bids_test/sub-PD01759D757223/ses-01.
WARNING: We cannot add the IntendedFor field: no fmap/ in /scratch/bids_test/sub-PD01759D757223/ses-01  

Interestingly, if I remove "--command populate-intended-for" option, as long as I have POPULATE_INTENDED_FOR_OPTS in my heuristics, the fmap jsons get populated correctly with "IntendedFor" field!

@pvelasco
Copy link
Contributor Author

pvelasco commented Apr 4, 2022

Hi @nikhil153,

The warning you receive when running with the --command populate-intended-for option means that there are no fmaps in that session folder. Did you run heudiconv (without the --command option) first?
The intended use, AFAIK, is to run it first without any --command, and then run the --command on the initially processed data.
From the help (note the instead):

  --command {heuristics,heuristic-info,ls,populate-templates,sanitize-jsons,treat-jsons,populate-intended-for}
                        Custom action to be performed on provided files
                        instead of regular operation.

As for the observation:

Interestingly, if I remove "--command populate-intended-for" option, as long as I have POPULATE_INTENDED_FOR_OPTS in my heuristics, the fmap jsons get populated correctly with "IntendedFor" field!

I can replicate the behavior and I'm looking into it.

@yarikoptic
Copy link
Member

If there is an issue/question -- please file a new issue (may be referencing this PR). This PR was merged and should RiP ;)

@yarikoptic yarikoptic added enhancement minor Increment the minor version when merged labels Apr 20, 2022
@github-actions
Copy link

🚀 PR was released in v0.11.0 🚀

@WangYunHong98
Copy link

Hi @pvelasco

I meet the same issue that ShimSetting in _phasediff.json is slightly different from ShimSetting in _bold.json.

For example,
in _bold.json, the ShimSetting is [2957, 4394, -6624, 354, -7, -55, -115, -26]
in _phasediff.json, the ShimSetting is [2965, 4393, -6624, 368, 13, -63, -91, -25]

So this slight difference will have an effect on func when it does SDC?

This is also causing heudiconv errors when adding IntendedFor to _phasediff.json file.

Best,
Yunhong

@pvelasco
Copy link
Contributor Author

pvelasco commented Apr 8, 2024

Hi @WangYunHong98,

Lately I switched jobs and now I'm working on different projects. I haven't been able to work on this project for about two years, but I think the way to make the parameter comparison less restrictive would be to add a relative tolerance in the call to np.allclose here.

That said, first principles say that if the ShimSettings for the acquisition you want to correct are not the same as for the field/distortion maps, the correction you are applying by using the field/distortion map will not be correct, since the B0 field affecting the distorted image is not the same that you measured with the field/distortion maps. (The exception being the first entry, which is a frequency offset and will result in an image shift in the readout direction, which can be corrected when you do motion correction.)

Some people smarter than me argue that using an approximate field correction is better than no field correction but, afaik, there have been no publication examining whether that's true or not, or what can be considered "slightly different parameters". So I would argue that a very good solution would be to have a configuration parameter somewhere that allows you to specify how restrictive you want to be, and let the user specify it.

Right now, I don't have the bandwidth to make those changes. Do you feel up to the task? 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement minor Increment the minor version when merged released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants