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

Make suffixes and datatypes configurable, if possible #405

Open
tsalo opened this issue Nov 15, 2023 · 3 comments
Open

Make suffixes and datatypes configurable, if possible #405

tsalo opened this issue Nov 15, 2023 · 3 comments
Labels
enhancement New feature or request

Comments

@tsalo
Copy link
Contributor

tsalo commented Nov 15, 2023

What would you like to see added in this software?

I am trying to replace an internal, outdated copy of SDCFlows in ASLPrep with the current version. However, since asl and m0scan are not supported suffixes for PEPOLAR or fieldmap-less distortion correction, I cannot use SDCFlows as a dependency without making changes to the codebase, getting those changes merged in, and getting a new release minted.

It seems like this restriction could be alleviated if suffixes and datatypes could be configured outside of the functions and classes.

Do you have any interest in helping implement the feature?

Yes

Additional information / screenshots

For example, there's a constant in sdcflows.fieldmaps called MODALITIES that I could override with a context manager when I use FieldmapFile in ASLPrep:

MODALITIES = {
"bold": EstimatorType.PEPOLAR,
"dwi": EstimatorType.PEPOLAR,
"epi": EstimatorType.PEPOLAR,
"fieldmap": EstimatorType.MAPPED,
"magnitude": None,
"magnitude1": None,
"magnitude2": None,
"phase1": EstimatorType.PHASEDIFF,
"phase2": EstimatorType.PHASEDIFF,
"phasediff": EstimatorType.PHASEDIFF,
"sbref": EstimatorType.PEPOLAR,
"T1w": EstimatorType.ANAT,
"T2w": EstimatorType.ANAT,
}

However, FieldmapFile then has suffixes hardcoded throughout, so, even if I modified MODALITIES it still wouldn't recognize the suffixes I need. For example:

# Check for REQUIRED metadata (depends on suffix.)
if self.suffix in ("bold", "dwi", "epi", "sbref"):
if "PhaseEncodingDirection" not in self.metadata:
raise MetadataError(
f"Missing 'PhaseEncodingDirection' for <{self.path}>."
)

@tsalo tsalo added the enhancement New feature or request label Nov 15, 2023
@effigies
Copy link
Member

Sounds completely reasonable. I think it will be worth sitting down and thinking through the API at some point, but it does not sound like this will significantly modify our API right now, so full steam ahead.

@tsalo
Copy link
Contributor Author

tsalo commented Nov 20, 2023

Once I get ASLPrep working with my branch of SDCFlows, I'll be able to open a PR.

@oesteban
Copy link
Member

This would be worth bringing up in a TechMon meeting.

@tsalo - the next one will be focused on multi-echo fMRIPrep, but we may have some time for this (at least comment it out so it reaches beyond those who read this issue).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants