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

support reinterpret from UIntX to Fixed{IntX, N} #96

Open
ssfrr opened this issue Oct 16, 2017 · 8 comments
Open

support reinterpret from UIntX to Fixed{IntX, N} #96

ssfrr opened this issue Oct 16, 2017 · 8 comments

Comments

@ssfrr
Copy link
Contributor

ssfrr commented Oct 16, 2017

Currently if you have a UInt16 that you want to reinterpret as a signed fixed-point number the reinterpret method here doesn't apply, so you get an error:

julia> reinterpret(Fixed{Int16, 15}, 0x8123)
ERROR: bitcast: target type not a leaf primitive type
Stacktrace:
 [1] reinterpret(::Type{FixedPointNumbers.Fixed{Int16,15}}, ::UInt16) at ./essentials.jl:155
 [2] eval(::Module, ::Any) at ./boot.jl:235

Is it worth having another reinterpret method to handle this, or is it safer to have the user double-reinterpret? (e.g. reinterpret(Fixed{Int16, 15}, reinterpret(Int16, 0x8123)))

One proposed method would be:

function Base.reinterpret(::Type{Fixed{T,f}}, x::Unsigned) where {T <: Signed,f}
    reinterpret(Fixed{T, f}, reinterpret(T, x))
end

Or we could even drop the requirement the x subtypes Unsigned if we want to reinterpret more broadly.

@kimikage
Copy link
Collaborator

kimikage commented Apr 22, 2020

If this is a problem with the redundancy, we can write it as:

julia> reinterpret(Q0f15, signed(0x8123))
-0.99112Q0f15

I don't think the implicit conversion is good from the viewpoint of preventing bugs.

On the other hand, I think there is still debate about the need for UnsignedFixed.(Edit: So, a loose reinterpret will cause confusion if Fixed{UInt16,f} is supported.) However, it is another matter.

@ssfrr, can I close this issue?
cc: @timholy

@timholy
Copy link
Member

timholy commented Apr 22, 2020

reinterpret(T, bits) should be independent of the type assigned to bits, as long as it's the right number of bits for T. For example:

julia> a = [0x8123]
1-element Array{UInt16,1}:
 0x8123

julia> reinterpret(Q0f15, a)
1-element reinterpret(Fixed{Int16,15}, ::Array{UInt16,1}):
 -0.99112Q0f15

julia> @which reinterpret(Q0f15, a)
reinterpret(::Type{T}, a::A) where {T, N, S, A<:AbstractArray{S,N}} in Base at reinterpretarray.jl:14

@kimikage
Copy link
Collaborator

kimikage commented Apr 22, 2020

It is documented that the behavior is different between "arrays" and others.

reinterpret(T, bits) should be independent of the type assigned to bits

That conflicts with the error message. 😕

ERROR: bitcast: target type not a leaf primitive type

I beleve that Julia's reinterpret should be safer than C++'s reinterpret_cast.

Edit:
Of course, it doesn't matter here whether it's a primitive type or not. The important point is that Julia uses a safe method.

@timholy
Copy link
Member

timholy commented Apr 22, 2020

It is documented that the behavior is different between "arrays" and others.

By my reading, that only documents the fact that it is implemented for arrays and not for other types. That error message comes from the fact that it fails the "has 0 fields test" here. AFAICT that's not a property that is important other than for implementation.

I believe that Julia's reinterpret should be safer than C++'s reinterpret_cast.

That's not unreasonable. reinterpret doesn't get used for dangerous stuff nearly as much as pointer casts in the C/C++ world. In other words, circumscribed usage is a more important form of safety than circumscribed types. Since it already is safer, do we need to add an additional layer of type-safety?

That said, I certainly don't feel strongly about this issue, to me it's a very niche issue and I can happily live with the status quo.

@kimikage
Copy link
Collaborator

Since it already is safer, do we need to add an additional layer of type-safety?

In essence I agree with you. I didn't mean "preventing bugs" to be "type-safe".
The reason I commented on this issue, submitted 2.5 years ago, was because I was just tackling JuliaGraphics/ColorTypes.jl#183 (comment). "If" I propose to support reinterpret(::Type{RGB24}, ::Float32), I think you'll frown.
As you know, I have no bones about reinterpreting Float32 <--> UInt32, so I'm not in a position to deny the proposal of OP, and I have no intention to strongly deny it.:smile:

@BioTurboNick
Copy link

Just stumbled across this, via a headache with loaded images. I have a PR over on Julia to allow general conversions between bits types of the same size. Which would solve this entire class of annoyances. Happy to entertain ways to make it fool-proof.

JuliaLang/julia#47116

@BioTurboNick
Copy link

Update: The PR I mentioned above is integrated into Julia as of 1.10, so this functionality is now available via Base.

@kimikage
Copy link
Collaborator

kimikage commented Jun 1, 2024

At first glance, this appeared to be solved.
However, thinking about it more carefully, it was more of a problem that was exposed.

In other words, previously, every downstream package did an explicit reinterpret because julia did not allow it. Now, downstream packages can do various reinterpretations, and that can cause compatibility bugs. Even though CI checks can prevent releasing the bugs, the ❌ sign makes the developers tired.

Of course, that is the downstream package's own responsibility.
But wouldn't it be beneficial in the JuliaGraphics and JuliaImages ecosystems to add compatibility support for typical cases?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants