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

FIX: Implement B0FieldIdentifier / B0FieldSource #247

Merged
merged 4 commits into from
Oct 27, 2021

Conversation

oesteban
Copy link
Member

Start with the implementation of the new BIDS metadata entries, to
facilitate more reliable estimation and correction with fieldmaps.

Resolves: #246.

Start with the implementation of the new BIDS metadata entries, to
facilitate more reliable estimation and correction with fieldmaps.

Resolves: #246.
@oesteban oesteban marked this pull request as ready for review October 26, 2021 16:34
Copy link
Contributor

@mgxd mgxd left a comment

Choose a reason for hiding this comment

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

I suggested some additional query arguments to ensure we only grab subject relevant files.

Will this require us to make changes to the current fmriprep / nibabies way of handling the B0* metadata?

sdcflows/utils/wrangler.py Outdated Show resolved Hide resolved
sdcflows/utils/wrangler.py Outdated Show resolved Hide resolved
sdcflows/utils/wrangler.py Outdated Show resolved Hide resolved
sdcflows/utils/wrangler.py Outdated Show resolved Hide resolved
sdcflows/utils/wrangler.py Outdated Show resolved Hide resolved
Co-authored-by: Mathias Goncalves <mathiasg@stanford.edu>
estimators.append(e)
b0_ids = tuple()
with suppress(BIDSEntityError):
b0_ids = layout.get_B0FieldIdentifiers(**base_entities)
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to specify extension and scope? I think only subject is relevant. Possibly session.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think extension would be required, yes. But, in the case of a weird dataset where pybids finds a json without its NIfTI this would fail for very little gain.

scope is necessary, because derivatives/ will definitely have B0FieldIdentifier defined if processed with SDCFlows.

subject is necessary, of course,

and session should not be there - the user may want to use fieldmaps with data from another session and save time in others (e.g., My Connectome).

Copy link
Member

Choose a reason for hiding this comment

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

Okay, as long as extension doesn't break things, that's fine.

Copy link
Member

Choose a reason for hiding this comment

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

scope is necessary, because derivatives/ will definitely have B0FieldIdentifier defined if processed with SDCFlows.

But suppose I have a qMRI sequence where I quantify B0, which would be a derivative, and want to use that to correct raw BOLD series?

Copy link
Member Author

Choose a reason for hiding this comment

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

If it is preprocessed, and in Hz, then you don't need an estimator for that - this function finds data that potentially can be used to estimate a fieldmap.

If, on the contrary, some preprocessing is needed (e.g., you will want to approximate some B-Spline coefficients to properly integrate with the correction workflows), then SDCFlows does not support that type of fieldmap. You would definitely pick up some sdcflows.interfaces but essentially, you have to build the workflow yourself. Also, our Fieldmap and FieldmapEstimator objects do not support other types of fieldmaps.

Copy link
Member Author

Choose a reason for hiding this comment

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

The responsibility of this function is merely implementing the best we can those heuristics we used to have in fMRIPrep, for convenience across the board (dMRIPrep, babies, rodents, etc.).

Copy link
Member Author

@oesteban oesteban Oct 27, 2021

Choose a reason for hiding this comment

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

Before I merge, I guess I didn't explicitly give an answer to the actual question.

But suppose I have a qMRI sequence where I quantify B0, which would be a derivative, and want to use that to correct raw BOLD series?

Then, (i) you want it to have a B0FieldIdentifier in it, or at the least the IntendedFor so that the code to find fieldmap matches locates it, (ii) if you want to use SDCFlows' unwarp workflow, then you need to make sure that an anatomical reference is given and B-Spline coefficients representation available - if not, that's something you'll write on your own, (iii) invoke the unwarp workflow.

As you will see with the links, both (i) and (iii) take place within the downstream library (i.e., not implemented in SDCFlows proper).

@oesteban oesteban merged commit afc110d into master Oct 27, 2021
@oesteban oesteban deleted the fix/b0field-identifier branch October 27, 2021 09:38
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.

Incorrect handling of new BIDS' B0FieldIdentifier
3 participants