search

aramis-lab / clinica

Software platform for clinical neuroimaging studies
http://www.clinica.run/
Other
227 stars 81 forks source link

Factorize `install_nifti` #826

Open NicolasGensollen opened 1 year ago

NicolasGensollen commented 1 year ago

Both converters habs2bids and nifd2bids have their own implementation of install_nifti:

https://github.com/aramis-lab/clinica/blob/83d6d9958b3b3e2ffe82e5fedf09cd70b3109a44/clinica/iotools/converters/habs_to_bids/habs_to_bids.py#L178-L185

https://github.com/aramis-lab/clinica/blob/83d6d9958b3b3e2ffe82e5fedf09cd70b3109a44/clinica/iotools/converters/nifd_to_bids/nifd_utils.py#L320-L328

I believe these could be unified and factored out (in bids_utils maybe ?).

Also, the name of the function could be changed to something more explicit.

ghisvail commented 1 year ago

One is a plain install from the filesystem, the other performs extraction and install from an archive.

It could be made functionally equivalent, I just wonder whether efforts would not be better spent transforming our converters to proper workflows. I believe better refactoring patterns may emerge from this. Maybe?

NicolasGensollen commented 1 year ago

Reducing the amount of unnecessary code can ease the refactoring. I think there are tons of duplicated logic in between converters and pipelines that could be factorized. This factorization would (maybe?) make similar patterns appear more clearly between converters and pipelines.

That being said, I just saw this particular example while looking for something else. As you said, it is not as straightforward as deleting one and importing the other, but I figured it could be a good low hanging fruit for interested new contributors.

github-actions[bot] commented 1 year ago

This issue is considered stale because it has not received further activity for the last 14 days. You may remove the inactive label or add a comment, otherwise it will be closed after the next 14 days.

github-actions[bot] commented 1 year ago

This issue is considered stale because it has not received further activity for the last 14 days. You may remove the inactive label or add a comment, otherwise it will be closed after the next 14 days.

github-actions[bot] commented 1 year ago

This issue is considered stale because it has not received further activity for the last 14 days. You may remove the inactive label or add a comment, otherwise it will be closed after the next 14 days.

github-actions[bot] commented 11 months ago

This issue is considered stale because it has not received further activity for the last 14 days. You may remove the inactive label or add a comment, otherwise it will be closed after the next 14 days.

github-actions[bot] commented 8 months ago

This issue is considered stale because it has not received further activity for the last 14 days. You may remove the inactive label or add a comment, otherwise it will be closed after the next 14 days.

github-actions[bot] commented 5 months ago

This issue is considered stale because it has not received further activity for the last 14 days. You may remove the inactive label or add a comment, otherwise it will be closed after the next 14 days.

github-actions[bot] commented 2 months ago

This issue is considered stale because it has not received further activity for the last 14 days. You may remove the inactive label or add a comment, otherwise it will be closed after the next 14 days.