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

Add option to control t2smap fittype #3029

Closed
tsalo opened this issue Jun 8, 2023 · 2 comments · Fixed by #3030
Closed

Add option to control t2smap fittype #3029

tsalo opened this issue Jun 8, 2023 · 2 comments · Fixed by #3030

Comments

@tsalo
Copy link
Collaborator

tsalo commented Jun 8, 2023

What would you like to see added in fMRIPrep?

Users have reported memory issues with the nonlinear T2*/S0 fitting setting (e.g., #2935 and #2827), so it might be worthwhile to give them control over the setting. I don't think it merits a command-line parameter, but is it perhaps something that could be controlled via the config file?

Do you have any interest in helping implement the feature?

Yes

Additional information / screenshots

Alternatively, I could change the fittype from nonlinear to the less memory-intensive, but potentially less accurate, loglin option.

@effigies
Copy link
Member

effigies commented Jun 8, 2023

I think it would be okay to add a parameter. --me-t2s-fit-method or something. I'd rather it be accessible by CLI than make people start caring about config files.

@tsalo
Copy link
Collaborator Author

tsalo commented Jun 8, 2023

Sounds good. I can open a PR for that.

effigies added a commit that referenced this issue Jun 9, 2023
Closes #3029.

## Changes proposed in this pull request

- Add a new command line parameter, `--me-t2s-fit-method`, to control
how T2* and S0 will be estimated with tedana. The default, "curvefit",
is slower and more memory intensive, which has led to out-of-memory
errors for some users. The new option, "loglin", shouldn't result in the
same memory issues, but it also may produce less accurate estimates.


## Documentation that should be reviewed


I added a couple of sentences on the new parameter to the "T2*-driven
echo combination" section of `workflows.rst`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants