-
Notifications
You must be signed in to change notification settings - Fork 11
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
Inverse Linear Transform on writing #28
Comments
And now we have a bug because of this problem! We tried saving a u16 image with a header from a reference anatomy with a complex signal&inter (SI), like S=0.182 and I = 491.54. It doesn't produce a black image, it crashes with a We discussed at Imeka and we have reached some conclusion. This problem doesn't affect float and rgb images, so lets forget about them. For all integer images (
So we could simply add Now that we understand that integer images don't need SI (if you agreed ^^), we can apply the same thought process to float image. Why would we (or anyone) want to apply a SI to a float image? It would simply cast it to the same type. The fact that we decide the on-disk datatype from the templated type makes SI totally useless for writing all image types. Of course, it's required and important for reading images. |
I think all this amounts to one of two choices when writing a file:
To be honest, I was hoping to see the latter happen when I posted that comment at #26. The reasoning be that:
I would call this a bug, rather than an issue with SI management. In practice, we would have 3 data types in this conversion: (1) the original data from the ndarray or in-mem volume However, considering that making this work is not very important right now, and that the current state of the library is a bit problematic, it's best that we impose the former approach to fix the "divide by zero" error, and perhaps support scaling at a later date. Can you write a PR for this? All I ask is that this behaviour is well document in the writer functions. |
Of course, everything I wrote was taking into account that From the reference header, like NiBabel? If so, I don't agree that it's "requested". In our case, we use a reference image only to ensure that we are in the right space (it's mostly an affine cloner), it's never to request a datatype. In fact, I (personally) always hated NiBabel behavior. Or directly from the user when calling write, like |
Oh, somehow I had the impression that our To sum up: let's override |
Perfect. Thanks for the discussion. I'll write a PR, probably next week. |
The current implementation of slope/inter is too simple and produces wrong results if
T
is an integer type and slope/inter are not exactly equal to an integer.For example, a slope of 0.5 on a
u16
image would produce a black image (filled with 0).We should use a concept similar to
data::element
. See this comment from @Enet4 for more information.The text was updated successfully, but these errors were encountered: