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.

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

Deal with nibabel deprecation of get_header and get_data #3879

Merged
merged 24 commits into from Sep 13, 2022

Conversation

mguaypaq
Copy link
Member

@mguaypaq mguaypaq commented Sep 7, 2022

Description

nibabel has deprecated the get_header() and get_data() methods, in favour of the header property and the get_fdata() method, respectively. The sct_dmri_denoise_patch2self.py script is one of the three callers of these methods in SCT, so it needs to be updated. And since it's such a small script, I decided to do a general cleanup while I was at it.

This is probably easier to review commit-by-commit, since each one is a single change.

I think the only controversial change is that I added the second output file to the display_viewer_syntax call at the end (which was possibly showing an incorrect filename anyway if -o wasn't used).

Linked issues

Part of #3854.

Except for examples which end in a filename, to avoid confusion.
It's all explained in the helps string.
There's no hdr_1, hdr_2, etc.
Since these are nibabel image objects and numpy arrays:

img -> nii
img_denoised -> nii_denoised
img_diff -> nii_diff

denoised -> data_denoised
diff_4d -> data_diff

According to the naming convention on the wiki:
https://github.com/spinalcordtoolbox/spinalcordtoolbox/wiki/Programming%3A-Naming-conventions-(variables,-files)
Also this brings the numpy array computations together, separate
from the "Save files" step.
@mguaypaq mguaypaq added refactoring category: improves code structure without affecting user-facing functionality sct_dmri_denoise_patch2self context: labels Sep 7, 2022
@mguaypaq mguaypaq added this to the 5.8 milestone Sep 7, 2022
@mguaypaq mguaypaq self-assigned this Sep 7, 2022
Copy link
Member

@joshuacwnewton joshuacwnewton left a comment

Choose a reason for hiding this comment

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

LGTM! The majority of my comments are mostly tangential discussions -- I don't really have any changes to request (and the few comments I had, you already fixed!).

spinalcordtoolbox/scripts/sct_dmri_denoise_patch2self.py Outdated Show resolved Hide resolved
spinalcordtoolbox/scripts/sct_dmri_denoise_patch2self.py Outdated Show resolved Hide resolved
spinalcordtoolbox/scripts/sct_dmri_denoise_patch2self.py Outdated Show resolved Hide resolved
spinalcordtoolbox/scripts/sct_dmri_denoise_patch2self.py Outdated Show resolved Hide resolved
spinalcordtoolbox/scripts/sct_dmri_denoise_patch2self.py Outdated Show resolved Hide resolved
spinalcordtoolbox/scripts/sct_dmri_denoise_patch2self.py Outdated Show resolved Hide resolved
spinalcordtoolbox/scripts/sct_dmri_denoise_patch2self.py Outdated Show resolved Hide resolved
spinalcordtoolbox/scripts/sct_dmri_denoise_patch2self.py Outdated Show resolved Hide resolved
@mguaypaq mguaypaq merged commit eb80678 into master Sep 13, 2022
@mguaypaq mguaypaq deleted the mgp/nibabel-deprecations branch September 13, 2022 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring category: improves code structure without affecting user-facing functionality sct_dmri_denoise_patch2self context:
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants