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

Upper limit of the number of fractional bits f #155

Closed
kimikage opened this issue Dec 25, 2019 · 13 comments
Closed

Upper limit of the number of fractional bits f #155

kimikage opened this issue Dec 25, 2019 · 13 comments

Comments

@kimikage
Copy link
Collaborator

kimikage commented Dec 25, 2019

In theory, the number of fractional bits f can be countlessly large, but what is the actual upper limit?
I assumed that it is 8sizeof(T)-1 for Fixed{T}, and 8sizeof(T) for Normed{T}.

However, the larger numbers are also used actually.

for (TI, f) in [(Int8, 8), (Int16, 8), (Int16, 10), (Int32, 16)]

F8 = Fixed{Int8,8}

x = Fixed{Int8,8}(0.3)

for T in (Fixed{Int8,8}, Fixed{Int16,8}, Fixed{Int16,10}, Fixed{Int32,16})

Perhaps some functions do not work properly with large f, due to implicit upper limits.

Edit:
I intend to support Fixed{T,f} where f == 8sizeof(T), i.e. throw no exceptions. However, they have no typealias and will not be tested enough.

@kimikage
Copy link
Collaborator Author

I also have some concerns about the type of f.

BTW, I think there is a problem in interoperability with specialization using integer literal parameters.

julia> Normed{UInt32,Int64(1)} === Normed{UInt32,Int32(1)}
false

julia> Normed{UInt32,Int64(1)} == Normed{UInt32,Int32(1)}
false

I don't know how serializers (e.g. JLD2) do (de-)serialization.

Originally posted by @kimikage in #129 (comment)

I think f <: Integer is fine except for the ambiguity associated with omitting constraints, but I think isa(f, Int) is problematic as a constraint.

isa(f, Int) || error("f must be an Int")

@timholy
Copy link
Member

timholy commented Dec 31, 2019

I think those test cases are broken, we should probably change them.

isa(f, Int) is problematic as a constraint.

Why?

@kimikage
Copy link
Collaborator Author

kimikage commented Jan 1, 2020

I think those test cases are broken, we should probably change them.

It is reasonable to consider them broken, but we need to be careful if they have been actually used. Should we add a depwarn as usual?
This decision affects the implementation of floor(::Fixed). I have implemented the code which may throw the exception (#158).

Why?

You will not be able to construct Normed{::T, ::Int32} on the 64-bit version, nor Normed{::T, ::Int64} on the 32-bit version.
If we can construct them, we can provide the methods for conversions, but if we can't, we can't even convert them.
We need to remember that N0f8 is heavily used as an element type. I think coexistence is the only way.

Edit:
Of course, Union{Int32, Int64} is also valid, but not useful as a constraint because the types cannot be unified.

@timholy
Copy link
Member

timholy commented Jan 1, 2020

You will not be able to construct Normed{::T, ::Int32} on the 64-bit version, nor Normed{::T, ::Int64} on the 32-bit version.

I'm not sure that's a problem. In the end types get identified as a pointer (see jltypes.c in Julia's src/ directory) and of course the compiler drops the types wherever possible, so a type-parameter is only needed essentially as a UUID. Is there a genuine use-case for non-Int f?

@kimikage
Copy link
Collaborator Author

kimikage commented Jan 1, 2020

I've been discussing the interoperability between the 32-bit and 64-bit systems since #129 (comment). If it doesn't matter, it's all fine.

Edit:
For example:

julia> versioninfo()
Julia Version 1.0.5
Commit 3af96bcefc (2019-09-09 19:06 UTC)
Platform Info:
  OS: Windows (i686-w64-mingw32)
  CPU: Intel(R) Core(TM) i7-8565U CPU @ 1.80GHz
  WORD_SIZE: 32
  LIBM: libopenlibm
  LLVM: libLLVM-6.0.0 (ORCJIT, skylake)

julia> using JLD2, FileIO, TestImages

julia> img = testimage("cameraman");

julia> @save "cameraman.jld2" img
julia> versioninfo()
Julia Version 1.3.1
Commit 2d5741174c (2019-12-30 21:36 UTC)
Platform Info:
  OS: Linux (x86_64-pc-linux-gnu)
  CPU: Intel(R) Core(TM) i7-8565U CPU @ 1.80GHz
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-6.0.1 (ORCJIT, skylake)

julia> using JLD2, FileIO, ColorTypes

julia> @load "cameraman.jld2" img;

julia> typeof(img)
Array{Gray{Normed{UInt8,8}},2}

julia> img[1,1]
Gray{N0f8}(Error showing value of type Gray{Normed{UInt8,8}}:
ERROR: f must be an Int

Of course, there is no need to save 2D images in JLD2. This is just an example.

Most functions should work with f in both Int32 and Int64 for now, since I avoided specialization with integer literals. The only specialization with integer literals is:

N0f16(x::N0f8) = reinterpret(N0f16, convert(UInt16, 0x0101*reinterpret(x)))

Perhaps adding promotion rules (Int32 or Int64 -> Int) will complete the interoperability. Well, we don't aim to achieve the interoperability, though.

@timholy
Copy link
Member

timholy commented Jan 1, 2020

Now I see what you mean. This really seems like something JLD2 should handle, and not be a problem that individual packages have to worry about. IMO any time there's an integer type parameter it should just be ::Int.

@kimikage
Copy link
Collaborator Author

kimikage commented Jan 1, 2020

It is definitely a good thing that JLD2 can handle it well. However, JLD2 is not the only serializer, and FixedPointNumbers is a somewhat famous but ordinary package, and not included in the standard library.

I agree with the principle. The dimension of Array also follows the principle. However, when f is not Int, there should be some reasons. I don't know whether the reasons are worth breaking the principle.

In any case, even if we restrict f into Int, we need to implement functions to work whether f is an Int32 or Int64.

@kimikage
Copy link
Collaborator Author

kimikage commented Jan 8, 2020

Is the following customization really useful? (I skeptically wrote this. )

function deserialize(s::AbstractSerializer, ::Type{Normed{T,f}}) where {T, f}
    tag = read(s.io, UInt8)
    reinterpret(Normed{T, Int(f)}, read(s.io, T))
end

One of the troubles of customizing Serialization is that container types (e.g. arrays) are specialized.

From an implementation point of view, I think it is simpler to use the promotion mechanism.

@timholy
Copy link
Member

timholy commented Jan 10, 2020

Hard to say in the abstract. You've probably seen what happens right now, but I haven't poked at this issue. If you need input, a set of failing tests would be a good starting point. (I don't have access to a 32-bit machine, however.)

@kimikage
Copy link
Collaborator Author

I don't have access to a 32-bit machine, however.

I don't want to add isa(f, Int) to the constructors "for now", so we can reproduce the types for 32-bit systems on 64-bit systems with explicit notations like Normed{UInt8, Int32(8)}.

BTW, I also think AppVeyor supports the 32-bit Windows build. Travis-ci also supports the 32-bit Linux build, but I have never tried it.
cf. https://discourse.julialang.org/t/32-bit-linux-julia-builds-are-now-available-on-travis-ci/27786

I don't think isa(f, Int) == false is a fatal problem, so I'd like to tackle it after the upcoming set of fixes has been completed.

@timholy
Copy link
Member

timholy commented Jan 10, 2020

I'm in no hurry, but I do think forcing Int is a useful normalization:

julia> using FixedPointNumbers

julia> T1 = Normed{UInt8,8}
Normed{UInt8,8}

julia> T2 = Normed{UInt8,Int32(8)}
Normed{UInt8,8}

julia> T1 == T2
false

julia> x = T2(0.4)
0.4N0f8

julia> isa(x, N0f8)
false

That's really confusing.

EDIT: but please don't try to fix this by changing how types are printed. There's a long and glorious history of hard-to-debug segfaults in Julia because of well-meaning customizations of type printing. (Customizing show for values, of course, is fine.)

@kimikage
Copy link
Collaborator Author

That's really confusing.

I completely agree. I also think "forcing" Int is useful. The problem is the way to force. As you know, isa(f, Int) || error() explains what happened, but does not help further to handle data. The matter is that the targets are constructors, not factory methods.

@kimikage
Copy link
Collaborator Author

PR #159 was merged, and I submitted a new issue (#162). So, let's continue the discussion there.

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

2 participants