-
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
[REF] Refactor and clean utils.input_files
module
#1311
base: dev
Are you sure you want to change the base?
[REF] Refactor and clean utils.input_files
module
#1311
Conversation
f3c8405
to
f4cc15b
Compare
1f08ae1
to
1c9d3d5
Compare
@AliceJoubert I think this is ready for review. To be honest, I'm not fully convinced by the factory pattern I ended up using. I feel that it is a bit too complex compared to directly importing the pattern getter functions. One of the main issue I see with my proposal is that when you do pattern = query_pattern_factory(QueryPatternName.T1W_LINEAR)(...) The IDE is not able to tell you the expected arguments of the pattern getter that the factory is returning. So you basically have to find it and check its signature to know about it, which isn't great... Also, considering our hackathon project, this code will be moved out of Clinica soonish and is likely to be refactored further. Having a dedicated library is one more reason to have the pattern getters as public functions of the API, since users might want to check their doc page. Let me know what you think. |
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.
Thanks for the work @NicolasGensollen ! I agree with you, it's important to not be obligated to search in the code for the input types.
Maybe with the hackathon we will decide on a structure that will be easier to factorize and use in a factory instead of importing all the functions. Anyways, it will be redefined at that moment so it is fine if this PR is not optimal.
I would vote for importing the functions right away and not use the factory 🙏
"generated with t1-freesurfer-longitudinal." | ||
), | ||
"t1-freesurfer and t1-freesurfer-longitudinal", | ||
) | ||
|
||
|
||
def aggregator(func): |
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.
The description for this function needs to be changed a bit since t1_volume_native_tpm
was removed. I cannot ping you at the right lines with the diff but all the examples here use t1_volume_native_tpm
.
4ff389f
to
c0a8b86
Compare
4b32dc9
to
f677c43
Compare
Closes #1308