Skip to content

Conversation

@timholy
Copy link
Member

@timholy timholy commented Sep 11, 2016

Gracious me, coding by copy/paste does have its downsides.

This one was my fault from back in 2014, I believe. Amazing it's not been noticed before.

@vchuravy
Copy link
Collaborator

I am not on a computer right now so I couldn't just test this :)

Was the problem here the return type of the div operator? It seems odd to me that the result would be an integer (in all cases).

@timholy
Copy link
Member Author

timholy commented Sep 12, 2016

This is what happens on master:

julia> using FixedPointNumbers
INFO: Recompiling stale cache file /home/tim/.julia/lib/v0.5/FixedPointNumbers.ji for module FixedPointNumbers.

julia> x = 16uf8
UFixed{UInt8,8}(0.063)

julia> y = 2uf8
UFixed{UInt8,8}(0.008)

julia> div(x, y)
UFixed{UInt8,8}(0.031)

julia> div(Float32(x), Float32(y))
8.0f0

julia> r = 1uf8:1uf8:10uf8
UFixed{UInt8,8}(0.004):UFixed{UInt8,8}(0.004):UFixed{UInt8,8}(0.039)

julia> length(r)
------ InexactError -------------------- Stacktrace (most recent call last)

 [1] — length(::StepRange{FixedPointNumbers.UFixed{UInt8,8},FixedPointNumbers.UFixed{UInt8,8}}) at range.jl:364

 [2] — unsafe_length(::StepRange{FixedPointNumbers.UFixed{UInt8,8},FixedPointNumbers.UFixed{UInt8,8}}) at range.jl:361

InexactError()

None of those problems happen with this branch.

@vchuravy
Copy link
Collaborator

Hm I see. I am still trying to wrap my head around this. Isn't the problem
that the result of the division is not representable as a UF8? I am still
wondering how falling back to integer division solves this, wouldn't it
make more sense to fall back to floating point division?

Can I also note that the printing of 2uf8 is broken on master? Or the
conversion to Float32.

On Sun, 11 Sep 2016, 21:14 Tim Holy, [email protected] wrote:

This is what happens on master:

julia> using FixedPointNumbers
INFO: Recompiling stale cache file /home/tim/.julia/lib/v0.5/FixedPointNumbers.ji for module FixedPointNumbers.

julia> x = 16uf8
UFixed{UInt8,8}(0.063)

julia> y = 2uf8
UFixed{UInt8,8}(0.008)

julia> div(x, y)
UFixed{UInt8,8}(0.031)

julia> div(Float32(x), Float32(y))8.0f0

julia> r = 1uf8:1uf8:10uf8
UFixed{UInt8,8}(0.004):UFixed{UInt8,8}(0.004):UFixed{UInt8,8}(0.039)

julia> length(r)------ InexactError -------------------- Stacktrace (most recent call last)

[1] — length(::StepRange{FixedPointNumbers.UFixed{UInt8,8},FixedPointNumbers.UFixed{UInt8,8}}) at range.jl:364

[2] — unsafe_length(::StepRange{FixedPointNumbers.UFixed{UInt8,8},FixedPointNumbers.UFixed{UInt8,8}}) at range.jl:361
InexactError()

None of those problems happen with this branch.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#47 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAI3aoPhk_fZTORDySMnW9r5KoX6YV6wks5qpKdXgaJpZM4J6Igf
.

for f in (:div, :fld, :rem, :mod, :mod1, :rem1, :fld1, :min, :max)
for f in (:div, :fld, :fld1)
@eval begin
$f{T<:UFixed}(x::T, y::T) = $f(reinterpret(x),reinterpret(y))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what confuses me. The result type here is now an unsigned integer.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you arguing we should throw an InexactError?

@timholy
Copy link
Member Author

timholy commented Sep 12, 2016

Yes, it's not representable, but in general you expect it not to be. The definition of div is just "Computes x/y, truncated to an integer." So the result should be 8, not 0.031.

I'm not sure I follow what's broken about the printing/conversion to Float32:

julia> x = 2uf8
UFixed{UInt8,8}(0.008)

julia> Float32(x)
0.007843138f0

(except I'm bummed about how verbose the printing is, I thought we'd fixed that)

@vchuravy
Copy link
Collaborator

Ahhh, I am too tired. You are of course right.

  • div should return integers.
  • and I compared Float32(2uf8) to the wrong number

Thanks for explaining. LGTM from my side.

@timholy timholy merged commit 65b5c31 into master Sep 12, 2016
@timholy timholy deleted the teh/fix_div branch September 12, 2016 11:36
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

Successfully merging this pull request may close these issues.

3 participants