[RFC/WIP] Homogenise design

[vchuravy]

So I am currently working on reworking the design of FixedPointNumbers so theUFixed and Fixed work the same way. Currently this conversion is not yet fully done and I would like some early feedback on the idea. (The overall goal is to make #25 possible)

Type Hierarchy

abstract FixedPoint{T <: Integer, B, f} <: Real
immutable UFixed{T<:Unsigned, B, f} <: FixedPoint{T, B, f}
  i :: T
end
immutable Fixed{T <: Signed,B,f} <: FixedPoint{T, B, f}
  i :: T
end

• T Is the underlying type
• B Is the bits available for usage (To make 10 Bit numbers et. possible)
• f Is the fractional part of the B bit integer

This leads to the fact that the types Colors is using are now defined as ``typealias UFixed8 UFixed{UInt8,8,8}since they are expected to be normalised. That is the place where currentlyUFixed` and `Fixed32` feel the most disjoint and it may warrant the introduction of a normalised `FixedPoint` type.

Rename

In #16 it was proposed to change Ufixed to UFixed following the changes of Base.

Fallout

The heaviest user of FixedPointNumbers as far as I am aware of is the Colors ecosystem. I am willing to help resolve any issue that might come up. Also I don't know how one would deprecate the functionality nicely and it might be easier to create this first in a new package.

Todo

• Implement fixed point math
• Check compability with current packages
• Add deprecations for type names/ typealiase
• Documentation
• More test cases

The current set of changes is incomplete, since I would rather have feedback before I spend to much time on this.

@timholy @JeffBezanson

[timholy]

👍

But, do you need the B parameter? How would it be used? Currently a Ufixed12 satisfies typemax(Ufixed10) = Ufixed10(64.0616), because you can use the top 6 bits to represent numbers bigger than 1. I don't know whether you'd think of that as a good thing or a bad thing.

[vchuravy]

I think it is a good thing to use those extra bits for computation, but if I ask a Ufixed for its value there should be a guarantee that it doesn't exceed it type max.
Currently I have not implemented that part yet. (I would do that in the convert functions)

B Is so that we are able to say things like: "UFixed{UInt16,10,2} represents a number that has 8 bits for integer part and 2 bits for its fractional part" That is the way Y'CbCr10 is defined in ITU-R BT.601-7

@timholy I changed the multiplication for UFixed numbers to be

 UFixed{T,B,f}((Base.widemul(x.i,y.i) + (convert(widen(T), 1) << (f-1) ))>>f,0)

Along the lines of how Fixed works. That also guarantees that arithmetic operations always return UFixed types, but the error bounds in the test are then no longer accurate.

[timholy]

but if I ask a Ufixed for its value there should be a guarantee that it doesn't exceed it type max.

Currently the design is to say that those extra bits are perfectly acceptable, and that they are part of its typemax. If you want to impose 0 <= value(x) <= 1, I think that's a new notion. To make it composable, perhaps we want

immutable BoundedNumber{T,MinVal,MaxVal} <: Number
    value::T
end

and then any numeric type can become a BoundedNumber.

[timholy]

👍 on the multiplication change.

[vchuravy]

Hm I see. So in the original design UFixed{UInt16, 16, 10}, UFixed{UInt16, 12, 10} and UFixed{UInt16, 10, 10} are the same thing?

I think we could get away with that. That is effectively how this PR is implementing it right now. I see where my miss understanding comes from. I always took UFixed10 to say: "This is a 10 bit number normalized to 0..1", but actually it means "This is a unsigned 16 bit number with a 10 bit fractional part".

Coming back to my motivating case. Y'CbCr requires a (signed/unsigned) 10 bit number with 2 bit for the fractional part or a 12 bit number with 4 bit for the factional part.

Currently I could just drop the B and the PR would still be in good shape and I would have to take care of the clipping/limits in ColorTypes and we can talk later about BoundedNumber.

[timholy]

That would be fine.

[vchuravy]

Ok done. Any ideas how to handle the tests?

[timholy]

As in, the failing tests? How about wrapping the Float32 result in oftype(x*y, <Float32 result>)?

[vchuravy]

@timholy adding the deprecations makes ColorTypes pass locally. The only question is how to do that on julia versions before v0.4-rc2

[timholy]

Probably easiest is to skip the deprecation and just define const Ufixed8 = UFixed8 etc.

[timholy]

Not sure whether you're ready to merge yet (title might need updating?), but this looks really good to me. Thanks for tackling a much-needed cleanup.

On at least one point I owe you an apology: perhaps could we do a version check VERSION >= v"0.4.0-rc2", and if true use the @deprecate_binding mechanism? Sorry to change my mind on this point.

One further thought: on 0.3, x::Uint8 + y::Uint8 returns a Uint. Given that this PR adopts the 0.4 convention, that might be viewed as inconsistent on 0.3. Shall we just bump the minimum required julia version to 0.4- with this PR?

Just ping again when you're ready to merge.

[vchuravy]

I think bumping the minimum required version is a good idea.

I am currently rewriting the readme and want to add more tests to this.

An alternative idea I just had is that the implementation of UFixed and Fixed don't really differ any more and maybe there is no reason for having different types?

We could have one Fixed type and replace the macros with @generated, but maybe that refactor should happen after merging this.

[timholy]

Sounds good. I too noticed that the distinction seemed likely to be irrelevant, but I agree that can happen as a second stage.

I'm going to go ahead and merge #30, as another PR is waiting on it. Hope it doesn't cause you any rebasing headaches.

[vchuravy]

@timholy I squashed and rebased everything. The test suite got a bit bigger ;)

edit: Failure on Nightly is

INFO: Cloning METADATA from https://github.com/JuliaLang/METADATA.jl
ERROR: GitError(Code:ECERTIFICATE, Class:SSL, The SSL certificate is invalid)

[timholy]

UInt8, not Uint8

[timholy]

Looks great!

The biggest problem is that the tests are timing out on every platform. I think we're going to have to add a section that's something like this (warning: untested) to .travis.yml:

script:
    - if [[ -a .git/shallow ]]; then git fetch --unshallow; fi
    - julia -e 'Pkg.clone(pwd()); Pkg.build("FixedPointNumbers")'
    - julia -e 'cd(Pkg.dir("FixedPointNumbers", "test")); include("runtests.jl")'

so the tests run with inlining on.

[vchuravy]

Let's see if that helps to speed up the travis run, otherwise we will have to reduce the number of tests for Fixed.

[timholy]

Doesn't seem to be working. I don't understand why---the old tests ran much faster on my machine. Or is this a consequence of the new tests? If on your own machine the tests are >10 minutes, that seems a little long.

If it should be better than this, maybe debug with changing that last line of the script section to

- julia -e 'fn = fieldnames(Base.JLOptions); for f in fn println(f, " => ", getfield(Base.JLOptions(), f)); end; cd(Pkg.dir("FixedPointNumbers", "test")); include("runtests.jl")'

This will cause it to dump the current options in a nicely-readable way. (can_inline is the one I'm most concerned about, but might as well see them all.)

[vchuravy]

The test take about 30min to run on my machine

INFO: FixedPointNumbers tests passed
1551.492159 seconds (447 allocations: 24.578 KB)

I will remove a few of the Fixed tests.

[vchuravy]

That seems to have worked. There are still more test cases then there used to be so merge whenever you are ready.

[timholy]

Awesome, thanks for your efforts here!

[vchuravy]

Thanks for guiding me along :)

Next step when I find sometime is to unify the code altogether.