Skip to content

[FEAT] Allow for custom topup config in PEPOLAR workflow#533

Closed
joey-scanga wants to merge 6 commits intonipreps:mainfrom
joey-scanga:custom_topup_conf
Closed

[FEAT] Allow for custom topup config in PEPOLAR workflow#533
joey-scanga wants to merge 6 commits intonipreps:mainfrom
joey-scanga:custom_topup_conf

Conversation

@joey-scanga
Copy link
Copy Markdown
Contributor

Tested with my fork of Nibabies master (in which I'm adding a flag --topup-config)

@joey-scanga joey-scanga changed the title Allow for custom topup config in PEPOLAR workflow [FEAT] Allow for custom topup config in PEPOLAR workflow Feb 20, 2026
Copy link
Copy Markdown
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.

Looks reasonable to me, just a few things left to address

Comment thread sdcflows/workflows/base.py Outdated
Comment thread sdcflows/workflows/base.py Outdated
Comment thread sdcflows/workflows/base.py Outdated
Comment on lines +111 to +112
if topup_config is None:
topup_config = str(data.load(f'flirtsch/b02b0{"_quick" * sloppy}.cnf'))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I wonder if we should add some text the workflow desc to highlight an alternative config was used

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Does cba9118 work? Should we add a copy to the fmap derivatives folder too?

joey-scanga and others added 5 commits March 5, 2026 10:42
Co-authored-by: Mathias Goncalves <goncalves.mathias@gmail.com>
Co-authored-by: Mathias Goncalves <goncalves.mathias@gmail.com>
Co-authored-by: Mathias Goncalves <goncalves.mathias@gmail.com>
Copy link
Copy Markdown
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.

LGTM, small suggestion

if topup_config is None:
topup_config = str(data.load(f'flirtsch/b02b0{"_quick" * sloppy}.cnf'))
else:
workflow.__desc__ += f' A custom `topup` configuration file was used: {topup_config}'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

since this could be a long filepath, i'm inclined to just say

Suggested change
workflow.__desc__ += f' A custom `topup` configuration file was used: {topup_config}'
workflow.__desc__ += ' A custom `topup` configuration file was used.

@joey-scanga
Copy link
Copy Markdown
Contributor Author

I think I deleted these changes when trying to fork from an older version for testing purposes (2.10.0), should I just redo this off the main branch?

@mgxd
Copy link
Copy Markdown
Contributor

mgxd commented Apr 16, 2026

sure

mgxd pushed a commit that referenced this pull request Apr 17, 2026
* Changed default topup config

* custom topup config files can now be used

* revert to old config
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.

2 participants