-
Notifications
You must be signed in to change notification settings - Fork 35
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
InexactError: Int16(..) when accessing NIfTI with rescale_slope #64
Comments
Experiencing the same error. Current workaround is manually scaling the data as follows: scaled_data = ni.raw .* ni.header.scl_slope .+ ni.header.scl_inter Any updates on fixing this @Tokazama ? |
Is anyone depending on rescaling? It's really just a bad idea to have as part of the standard and I think it causes a lot of unintuitive problems for users. I could just make it an opt-in parameter when loading. |
It isn't important for the magnitude really but it's pretty essential for phase (i.e phase in Integers is nonsensical). For this case,either the researcher must do the scaling or the scaling is handled properly by NIfTI.jl. It might make sense to have an option to read the NIVolume in as a specific type (i.e Float32, Float64, etc...), and then an InexactError would be at least informative. |
Yeah, and the use of anything other than 32 and 64 bit values will definitely be problematic for anyone trying to get performance for any serious computational work. I'm assuming the original intent behind automatically scaling values was to help researchers who didn't know anything about data types but wanted to save memory when storing data. The natural way to handle this in Julia is to just to do I'm not sure if there's a simpler way to support that in the load argument though. What does your typical workflow look like? |
I found the bug. It's a bug in the getindex method specific to the NIVolume object. The base getindex() method returns an array which is by default the same type as the elements passed into it, in our case f.raw, which can be a different type than f.header.scl_inter or f.header.scl_slope. See line 285 in src/NIfTI.jl. Either this line should be changed to avoid the type incompatibility, or (in the interest of simplicity) the functionality of indexing into the NIVolume should be removed and we should promote and document rescaling of the values independently of the data loading. |
The problem that we have right now seem quite common to me. occurring whenever you have a Int16 NIfTI file with scaling. For a quick data analysis script, it is quite ok to do external scaling, although confusing for newcomers to be greeted with an error message. For processing pipelines, this case has to be always considered and specially handled, or rather the implicit scaling should never be used. This is quite inconvenient and error prone at the moment. In this pull request, I had a go at this with a simple solution, by decoupling the NIVolume type from the type of the raw volume. The only place to adjust was the niupdate function to set the type in the nifti header to the I think this change wouldn't worsen any workflow. The main difference would be that Int16 files, when accessed via indexing, i.e. ni_image[:,:,:] return a Float32 image instead of Int16. In my opinion this makes most sense, since in the general case there is a scaling factor that is a float as well. If the underlying datatype is required, it makes sense to access it via ni_image.raw. The biggest disadvantage would probably be that it might be a breaking change, it could either be 0.6 or 1.0, since it has been stable for quite some time? |
Thanks for merging! |
@alexjaffray Does this solution also seem fine to you? |
If this solution works for everybody I'll get the release out today |
This solution works for me, thanks! @korbinian90 @Tokazama
Alex
…On Wed, 21 Jun 2023 at 19:04, Korbinian Eckstein ***@***.***> wrote:
@alexjaffray <https://github.com/alexjaffray> Does this solution also
seem fine to you?
@Tokazama <https://github.com/Tokazama> Can we go ahead with releasing
this as version 0.6?
—
Reply to this email directly, view it on GitHub
<#64 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEFRPBMIYP3KN5G32OSVV73XMOR3JANCNFSM6AAAAAAQVDH6YE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Since the 0.5.8 release, the addition of Int16 has somehow corrupted the previously working behavior.
From reading an Int16 file, the datatype
NIVolume{Int16, 4, Array{Int16, 4}} <: AbstractArray{Int16, 4} <: Any
is created. Previously, it was
NIfTI.NIVolume{Float32, 4, Array{Int16, 4}} <: AbstractArray{Float32, 4} <: Any
Reading the Int16 array with a scaling factor seems to violate the
AbstractArray{Int16, 4}
type hirarchy and an InexactError is thrown.Steps to reproduce:
The text was updated successfully, but these errors were encountered: