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

Avoid nibabel crashes due to int64 arrays by explicitly passing dtype/header to Nifti1Image #4433

Merged
merged 4 commits into from Apr 15, 2024
Merged

Avoid nibabel crashes due to int64 arrays by explicitly passing dtype/header to Nifti1Image #4433

merged 4 commits into from Apr 15, 2024

Conversation

joshuacwnewton
Copy link
Member

@joshuacwnewton joshuacwnewton commented Apr 12, 2024 •
edited

Description

nibabel will throw an error for instances of Nifti1Image(data, affine) when data is of type
int64 (and no header/dtype arguments are specified):

File "/home/runner/sct_6.3.dev0/testing/api/test_deepseg_sc.py", line 85, in test_uncrop_image
    nii = nib.nifti1.Nifti1Image(data_in, affine)
  File "/home/runner/sct_6.3.dev0/python/envs/venv_sct/lib/python3.9/site-packages/nibabel/nifti1.py", line 1836, in __init__
    alert_future_error(
  File "/home/runner/sct_6.3.dev0/python/envs/venv_sct/lib/python3.9/site-packages/nibabel/deprecated.py", line 120, in alert_future_error
    raise error_class(f'{msg} {error_rec}'.strip())
ValueError: """Image data has type int64, which may cause incompatibilities with other tools. 
               To use this type, pass an explicit header or dtype argument to Nifti1Image()."""

To avoid these errors, I did the following:

  • Did a project wide search (sans testing/) for instances of Nifti1Image (15 results)
  • Identified instances where data and affine were specified without header/dtype (7 results)
  • Identified which of these instances cannot have int64 data (3/7 results), and added comments for them.
  • For the remaining 4/7 instances, I either specified header or dtype, depending on context. (I used header when simply converting Image objects to Nifti1Image objects, and used dtype for a single dummy image usage.)

Linked issues

Fixes #4420.
Fixes #4408. (The errors, at least!)

This PR doesn't do anything to heed nibabel's warning, though. (It is still possible to generate int64 images using SCT.) However, I don't think int64 images are actually explicitly created anywhere by SCT? I think they only ever occur when a user passes int64 images as input. Given that, I think it's OK to close the underlying issue?

@joshuacwnewton
Add comment for instances where data cannot be int64
f2e5439 
nibabel will throw an error for instances of `Nifti1Image(data, affine)` when data is of type
int64 (and no header/dtype arguments are specified).

However, for these 3 instances of `Nifti1Image(data, affine)`, it is impossible for the array
to be of type int64 (due to both np.zeros and img.get_fdata returning float arrays). So, instead
of rewriting the calls to specify `dtype` or `header`, I've just added a little note.
@joshuacwnewton
slice.py: Specify header for calls to Nifti1Image
e04574e 
This ensures that an error won't be thrown if the image datatype is `int64`.

(Aside: I'm curious about the existence of `Slice._resample_slicewise()` when we already have an
 entire module dedicated to resampling in SCT.)
@joshuacwnewton
resampling.py: Specify header for call to Nifti1Image
5aa6ecb
@joshuacwnewton
resampling.py: Specify dtype arg for dummy 3D volumes
bc363f0 
Since we are creating dummy 3D volumes for resampling 4D images, I don't think it makes sense to
specify `header` here, since the only reference header we have is for the 4D image. So, instead,
we specify `dtype` to keep whatever dtype the 3D volume originally had.

Since the image will be resampled and stored into the `np.zeros` `data4d` object, I'm pretty sure
it will be coerced into a float type anyway, meaning that there is no risk of generating an int64
image here anyway.
@joshuacwnewton joshuacwnewton added bug category: fixes an error in the code sct_qc context: labels Apr 12, 2024
@joshuacwnewton joshuacwnewton added this to the 6.3 milestone Apr 12, 2024
@joshuacwnewton joshuacwnewton self-assigned this Apr 12, 2024
mguaypaq
mguaypaq approved these changes Apr 15, 2024
View reviewed changes
Copy link
Member

@mguaypaq mguaypaq left a comment

Choose a reason for hiding this comment

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

This change looks good, thanks. If the user explicitly asks for 64 bit integers, or is already working with 64 bit integers, it seems reasonable to let them.

If I had to guess, I'd say the fear of incompatibility might be a consequence of the fact that the NIfTI-1 format was designed to be sort-of-mostly readable by existing software as the older ANALYZE 7.5 format (by putting most of the relevant fields at the same byte offset in the header). But NIfTI-1 still added some new values for some of the existing enum fields. In particular, it added the signed and unsigned 64-bit integer data types (among others), which didn't exist before.

(For the list of added data types, see the official nifti.h C header file, and search for the string NIFTI1_DATATYPES.)

@mguaypaq mguaypaq merged commit 1333a1e into master Apr 15, 2024
20 checks passed
@mguaypaq mguaypaq deleted the jn/4408-avoid-nibabel-int64-crashes branch April 15, 2024 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mguaypaq mguaypaq mguaypaq approved these changes

@sandrinebedard sandrinebedard sandrinebedard approved these changes

Assignees

@joshuacwnewton joshuacwnewton

Labels
bug category: fixes an error in the code sct_qc context:
Projects
None yet
Milestone
6.3
3 participants
@joshuacwnewton @mguaypaq @sandrinebedard