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: Update eddy_params.json #903

Merged
merged 1 commit into from
Dec 10, 2024
Merged

ENH: Update eddy_params.json #903

merged 1 commit into from
Dec 10, 2024

Conversation

cookpa
Copy link
Collaborator

@cookpa cookpa commented Dec 10, 2024

Changes proposed in this pull request

Change the default eddy flm from linear to quadratic. This matches the eddy default.

The slm remains linear, as recommended for lower-resolution data.

Documentation that should be reviewed

I don't think the defaults are mentioned in the docs.

The default for eddy is flm: quadratic, slm: none. 

For modern data, flm: linear is not enough correction. The second level model is recommended for older and lower resolution data.
@cookpa
Copy link
Collaborator Author

cookpa commented Dec 10, 2024

Full disclosure, flm: quadratic / slm: linear is what I use, so here's my pitch.

@mattcieslak I know you mentioned using quadratic / quadratic but I looked through the FSL list and could not find any posts from the FMRIB group using anything other than linear for slm.

It might not make a big difference, but I figure it would be good for the defaults to follow the advice on the eddy user guide and mailing list, just so people don't worry about the difference.

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 44.23%. Comparing base (92687d1) to head (23a6104).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #903   +/-   ##
=======================================
  Coverage   44.23%   44.23%           
=======================================
  Files          66       66           
  Lines        9839     9839           
  Branches     1097     1097           
=======================================
  Hits         4352     4352           
  Misses       5263     5263           
  Partials      224      224           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@mattcieslak mattcieslak left a comment

Choose a reason for hiding this comment

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

I agree completely. Thanks for making the PR

@mattcieslak mattcieslak merged commit fa4dbe0 into PennLINC:master Dec 10, 2024
21 checks passed
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.

3 participants