Skip to content

Conversation

NicolasGensollen
Copy link
Member

@NicolasGensollen NicolasGensollen commented Oct 2, 2024

Closes #1308

@NicolasGensollen NicolasGensollen self-assigned this Oct 2, 2024
@NicolasGensollen NicolasGensollen force-pushed the replace-information-dict-with-query branch from f3c8405 to f4cc15b Compare October 3, 2024 10:26
@NicolasGensollen NicolasGensollen force-pushed the replace-information-dict-with-query branch from 1f08ae1 to 1c9d3d5 Compare November 6, 2024 10:29
@NicolasGensollen NicolasGensollen marked this pull request as ready for review November 8, 2024 12:12
@NicolasGensollen
Copy link
Member Author

@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.

Copy link
Contributor

@AliceJoubert AliceJoubert left a 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 🙏

)


def aggregator(func):
Copy link
Contributor

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.

@NicolasGensollen NicolasGensollen force-pushed the replace-information-dict-with-query branch from 4ff389f to c0a8b86 Compare November 13, 2024 12:59
@NicolasGensollen NicolasGensollen force-pushed the replace-information-dict-with-query branch from 4b32dc9 to f677c43 Compare November 14, 2024 10:14
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.

Improvements to module input_files

2 participants