-
Notifications
You must be signed in to change notification settings - Fork 82
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] Expose n_procs
option to Adni2BIDS
Oasis2BIDS
and Aibl2BIDS
#1009
[ENH] Expose n_procs
option to Adni2BIDS
Oasis2BIDS
and Aibl2BIDS
#1009
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This approach looks good and should work.
Let me propose a different alternative, which might reduce the amount of code duplication:
- Use Clinica's n_procs Click option wherever needed.
- Put the logic for computing the default value for
n_procs
in the default parameter for the option. - Expose parameterization with
n_procs
to converters with a safe default value of 1, instead of making it optional and duplicating calls toget_n_procs
to each pool. - Plugging the
n_procs
option to the parameter should provide the benefits of safe defaults inside Clinica's code, whilst keeping backward compatibility at the CLI layer.
Thanks for the review @ghisvail ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly what I had in mind 👌
I expected you would reuse the existing n_procs
in cli_param
to minimize the diff, but why not. I added a comment regarding the optional vs keyword param with default value.
clinica/iotools/converters/adni_to_bids/adni_modalities/adni_av45_fbb_pet.py
Outdated
Show resolved
Hide resolved
clinica/iotools/converters/adni_to_bids/adni_modalities/adni_av45_fbb_pet.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Ghislain Vaillant <[email protected]>
Thanks for the review ! 👍 |
Closes #1007