[REF] Refactor and clean utils.input_files module
#1311
Conversation
NicolasGensollen
force-pushed
the
replace-information-dict-with-query
branch
from
f3c8405
to
f4cc15b
Compare
NicolasGensollen
force-pushed
the
replace-information-dict-with-query
branch
from
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.
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.
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.
NicolasGensollen
force-pushed
the
replace-information-dict-with-query
branch
from
4ff389f
to
c0a8b86
Compare
NicolasGensollen
force-pushed
the
replace-information-dict-with-query
branch
from
4b32dc9
to
f677c43
Compare
Closes #1308