Skip to content

ENH: Switch major subworkflows params to inputs via a map of parameters#304

Merged
AlexVCaron merged 5 commits intonf-neuro:mainfrom
gagnonanthony:enh/switch_params_to_inputs
Feb 12, 2026
Merged

ENH: Switch major subworkflows params to inputs via a map of parameters#304
AlexVCaron merged 5 commits intonf-neuro:mainfrom
gagnonanthony:enh/switch_params_to_inputs

Conversation

@gagnonanthony
Copy link
Member

Type of improvement

If submitting a new module or fixing a bug, please use the appropriate template.

  • Documentation
  • Development tools (e.g. linter, formatter, etc.)
  • Development container
  • Global update (please specify)
  • Other (please specify)

Describe your improvement

The use of params within subworkflow were creating issues when we were trying to bind pipeline level parameters with subworkflow levels parameters. This PR switches all params.* within preproc_t1, preproc_dwi, topup_eddy, and tractoflow to a map of paramters supplied as an input called options.

Describe how to test your improvement

Run the tests

Checklist before requesting a review

  • Ensure the syntax is correct (EditorConfig and Prettier must pass)
  • Run the test suites if your changes affect any module
  • Regenerate the Poetry lock file if you have updated the dependencies
  • Ensure the documentation is up-to-date

@gagnonanthony gagnonanthony self-assigned this Feb 12, 2026
@gagnonanthony gagnonanthony requested a review from a team February 12, 2026 16:05
@gagnonanthony gagnonanthony requested review from AlexVCaron and arnaudbore and removed request for Manonedde and ThoumyreStanislas February 12, 2026 16:19
Copy link
Contributor

@AlexVCaron AlexVCaron left a comment

Choose a reason for hiding this comment

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

Major discussion point here, I see that we passed over it in recent PRs. What is expected to default to True, and what's not ? Now, with optionality, most preprocessing gets skipped (since null params evaluates to false). We should take a pause and fix that here

@gagnonanthony
Copy link
Member Author

gagnonanthony commented Feb 12, 2026

I think, since the optionality will now be documented in the meta.yml as an input, shouldn't it become the responsability of the pipeline dev to properly setup their inputs for the subworkflow (meaning the optionality that comes with it)? @AlexVCaron

I also think setting default can be done two ways:

  1. We do not set anything to true (as now), and users set them as they like/want.
  2. We set everything to true, and users disable what they don't want.

If we start mixing what is by default true and what's not, it can get more confusing rapidly.

arnaudbore
arnaudbore previously approved these changes Feb 12, 2026
Copy link
Contributor

@arnaudbore arnaudbore left a comment

Choose a reason for hiding this comment

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

GTG

@AlexVCaron AlexVCaron enabled auto-merge February 12, 2026 20:36
@AlexVCaron AlexVCaron added this pull request to the merge queue Feb 12, 2026
Merged via the queue into nf-neuro:main with commit a55b7b5 Feb 12, 2026
39 of 42 checks passed
@gagnonanthony gagnonanthony deleted the enh/switch_params_to_inputs branch February 12, 2026 22:00
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