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

Integer-based output images are unreadable with AFNI #757

Closed
tsalo opened this issue Jul 16, 2021 · 5 comments · Fixed by #759
Closed

Integer-based output images are unreadable with AFNI #757

tsalo opened this issue Jul 16, 2021 · 5 comments · Fixed by #759
Labels
bug issues describing a bug or error found in the project priority: high issues that would be really helpful if they were fixed already

Comments

@tsalo
Copy link
Member

tsalo commented Jul 16, 2021

Summary

@handwerkerd noticed in #736 that the adaptive mask output is unreadable in AFNI and MicroGL. This appears to be due to the fact that the reference image contains floats, so there may be a mismatch between the image header and the data.

This stems from #736 (comment).

Additional Detail

@jbteves thinks that new_img_like (nilearn's function) might be coercing the datatype incorrectly, so he will look into that element. If there is a bug on nilearn's side and we can get it fixed, then we'll need to update our requirements (and wait on the next nilearn release).

In the meantime, we will coerce the data to float and save that in the next release, then try to address the underlying issue for the release after that.

Next Steps

  1. Coerce img data to float in new_nii_like when creating new images as a patch for the next release.
  2. Look into saving integer imgs for the release after next.
@tsalo tsalo added bug issues describing a bug or error found in the project priority: high issues that would be really helpful if they were fixed already labels Jul 16, 2021
@jbteves
Copy link
Collaborator

jbteves commented Jul 16, 2021

Update: it's not nilearn's fault, however, it is due to something that nibabel does by default, which is to load things into float64 whenever get_fdata is called. So it's not a bug, it's a feature. Since the floating-points are 64-bit, when we cast our map to an integer amount, it will choose int64 instead of int32.
This also means that all of our files have been saving as float64, which most programs can read just fine. AFNI also just labels it float rather than float64, so there's no indication of the upscale for quite a few on the dev team unless you start looking at the raw headers or pay close attention to the fact that the files are big-looking.
The reason that this issue hasn't come up sooner is that nilearn will convert booleans to an int8 and our images have always been either boolean or floating-point before. There are basically two solutions:

  1. Read things as float32 instead of float64 since that's an overrideable default in get_fdata (see here). The clear advantage for this is that it will reduce memory footprint by about half, almost certainly speed things up, and makes sure our casts will be more compatible with other programs. The clear disadvantage is a loss of precision. It's not clear to me how much that precision matters, so I'd be curious to hear others' thoughts.
  2. Coerce the integer cast to always be int32 rather than int, which will upscale the values to be int64. The clear advantage is we retain precision, the clear disadvantages are both larger memory footprint and processing time.

Either fix is relatively straightforward since we have a centralized way of loading and saving files; we just have to read override the default for get_fdata in our loading function or add a check for int64 and downscale for the file saving function.
Tagging @emdupre and @handwerkerd in particular for their thoughts on how to proceed.

@handwerkerd
Copy link
Member

I think it's fine to READ float64, but maybe we should write as float32. That's similar to what AFNI does to reduce rounding error for the final bits in floating point data. int32 should be enough for anything we'd be using ints for. Maybe, in the short term, we can:

  • Read data with default settings, but only write data as int8 int32 or float32
  • Document this well so that it doesn't confuse future developers or users
  • If it becomes necessary, add some flexibility for other data types depending on the inputted data types

@eurunuela
Copy link
Collaborator

I completely agree with what @handwerkerd suggests. The loss in precision is, in my opinion, negligible and users will only notice the lighter file sizes.

@tsalo
Copy link
Member Author

tsalo commented Jul 17, 2021

Agreed. @handwerkerd your solution sounds good to me. Is anyone willing to tackle this in a new PR?

@jbteves
Copy link
Collaborator

jbteves commented Jul 19, 2021

Alright, I'll open a PR to implement @handwerkerd's suggestion today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug issues describing a bug or error found in the project priority: high issues that would be really helpful if they were fixed already
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants