Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[ENH][TEST] Move logic for file checking and add tests in center_nifti #1421

Merged
merged 7 commits into from
Jan 31, 2025
Merged

[ENH][TEST] Move logic for file checking and add tests in center_nifti #1421

merged 7 commits into from
Jan 31, 2025

Conversation

AliceJoubert
Copy link
Contributor

@AliceJoubert AliceJoubert commented Jan 28, 2025 •
edited
Loading

In this PR :

  • All the logic for file checking is moved in the private function for centering files
  • Unit tests are added
  • File overwriting was renewed :
    • Before : it copied only files at the root level (ex : description json) but not nifti files nested in folders if both directories had folders with same names. That is a bit misleading because it looks like it will preserve what is inside the BIDS input and actually... does not.
    • Now : will completely remove files inside the output folder.
      → This is a bit radical and could be changed for a solution that iteratively checks files from both folders in a follow-up PR

In another PR : non-regressions tests should look inside nifti files and check option for overwriting

@AliceJoubert AliceJoubert marked this pull request as ready for review January 30, 2025 16:50
NicolasGensollen
NicolasGensollen reviewed Jan 31, 2025
View reviewed changes
Copy link
Member

@NicolasGensollen NicolasGensollen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @AliceJoubert !

The PR LGTM. Just a few minor suggestions.

clinica/iotools/utils/data_handling/_centering.py Outdated Show resolved Hide resolved
test/unittests/iotools/utils/test_center_nifti.py Show resolved Hide resolved
test/unittests/iotools/utils/test_center_nifti.py Outdated Show resolved Hide resolved
clinica/iotools/utils/data_handling/_centering.py Outdated Show resolved Hide resolved
clinica/iotools/utils/data_handling/_centering.py Outdated Show resolved Hide resolved
clinica/iotools/utils/data_handling/_centering.py Outdated Show resolved Hide resolved
@AliceJoubert
Copy link
Contributor Author

Thanks for the review @NicolasGensollen ! Do you see anything else ?

NicolasGensollen
NicolasGensollen approved these changes Jan 31, 2025
View reviewed changes
Copy link
Member

@NicolasGensollen NicolasGensollen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @AliceJoubert !

@NicolasGensollen NicolasGensollen merged commit 1a8c456 into aramis-lab:dev Jan 31, 2025
12 of 14 checks passed
@AliceJoubert AliceJoubert deleted the centernif_move_logic branch January 31, 2025 10:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NicolasGensollen NicolasGensollen NicolasGensollen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@AliceJoubert @NicolasGensollen