Skip to content

fix isinteger #123

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.

Sign up for GitHub

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

Closed
wants to merge 10 commits into from
Closed

fix isinteger #123

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
5cf20d5
fix isinteger
johnnychen94 May 15, 2019
067dc1f
temp fix
johnnychen94 May 15, 2019
4bcc60c
Remove `N*f0` symbols (#121)
johnnychen94 May 15, 2019
279ea8a
enhance and export floattype (#122)
johnnychen94 May 15, 2019
0d72abf
make generate_fixedpoint_types a test utils
johnnychen94 May 16, 2019
8b19b8d
improve coverage of isinteger
johnnychen94 May 16, 2019
7ee8a04
Merge branch 'master' into isinteger
johnnychen94 May 16, 2019
e9b2da1
include utils.jl
johnnychen94 May 16, 2019
561eb94
change BigInt to rawtype
johnnychen94 May 16, 2019
d9a1201
disable unrelated CI
johnnychen94 May 16, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
0 .travis.yml → .travis.yml.bak
View file
Open in desktop
File renamed without changes.
6 changes: 3 additions & 3 deletions appveyor.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
environment:
matrix:
- julia_version: 1.0
- julia_version: 1
- julia_version: nightly
# - julia_version: 1
# - julia_version: nightly

platform:
- x86 # 32-bit
- x64 # 64-bit
# - x64 # 64-bit

## uncomment the following lines to allow failures on nightly julia
## (tests will run but not make your overall status red)
Expand Down
3 changes: 3 additions & 0 deletions src/normed.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,3 +169,6 @@ end

_unsafe_trunc(::Type{T}, x::Integer) where {T} = x % T
_unsafe_trunc(::Type{T}, x) where {T} = unsafe_trunc(T, x)

# predicates
isinteger(x::Normed{T,f}) where {T,f} = (x.i%(1<<f-1)) == 0
Comment on lines +173 to +174
Copy link
Collaborator

@kimikage kimikage Oct 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

See also comment for the Fixed test.
isinteger(x::Normed{T,f}) where {T,f} = (x.i%(one(T)<<f-1)) == 0
Edit:

Suggested change
# predicates
isinteger(x::Normed{T,f}) where {T,f} = (x.i%(1<<f-1)) == 0
isinteger(x::Normed{T,f}) where {T,f} = (x.i%(one(T)<<f-0x1)) == 0

FYI, (n%-1) == 0 is always true.

Moreover, if you write this here (I think we should do so), we should remove the following:

FixedPointNumbers.jl/src/FixedPointNumbers.jl

Lines 56 to 57 in da39318

# predicates
isinteger(x::FixedPoint{T,f}) where {T,f} = (x.i&(1<<f-1)) == 0

And then, add isinteger(x::Fixed{T,f}) where {T,f} = (x.i&(one(T)<<f-0x1)) == 0 into "src/fixed.jl"

Copy link
Collaborator

Choose a reason for hiding this comment

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

The above is analogous to isinteger(x::Fixed). Of course, you can use rawone(Normed{T,f}).

10 changes: 10 additions & 0 deletions test/fixed.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,16 @@ end
@test floatmin(Q11f4) == Q11f4(0.06)
end

@testset "isinteger" begin
# issue #120
for T in generate_fixedpoint_types(:Fixed)
T_ints = T.(clamp.(rand(rawtype(T), 500, 500),
ceil(rawtype(T), floattype(T)(typemin(T))),
floor(rawtype(T), floattype(T)(typemax(T)))))
@test all(isinteger.(T_ints))
end
end
Comment on lines +149 to +157
Copy link
Collaborator

@kimikage kimikage Oct 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

The reason for the failures on 32-bit systems is that the bit mask is only 32-bit length even for Fixed{Int64}.

FixedPointNumbers.jl/src/FixedPointNumbers.jl

Line 57 in da39318

isinteger(x::FixedPoint{T,f}) where {T,f} = (x.i&(1<<f-1)) == 0 

julia> typeof(1)
Int32

julia> typeof(1<<63)
Int32

julia> bitstring(1<<63-1)
"11111111111111111111111111111111"

julia> bitstring(one(Int64)<<63-1)
"0111111111111111111111111111111111111111111111111111111111111111"

Therefore, we should fix it up as follows:

isinteger(x::Fixed{T,f}) where {T,f} = (x.i&(one(T)<<f-0x1)) == 0

BTW, since this is a testset for Fixed, not Image, we do not have to store such a large matrix.
There are only 65,536(=256×256 < 500×500) different patterns for each Fixed{Int16} type. So, for Fixed{Int16} and Fixed{Int8} types, @timholy's method is better. (There is no round() for Fixed, though.) I think it is important to check the non-integer cases.
In addition, the types with a large f can represent only few integers (e.g. two integers: -1Q0f31 and 0Q0f31). I think this is too inefficient.


@testset "Disambiguation constructors" begin
@test_throws ArgumentError Fixed{Int32,16}('a')
@test_throws InexactError Fixed{Int32,16}(complex(1.0, 1.0))
Expand Down
10 changes: 10 additions & 0 deletions test/normed.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,6 +345,16 @@ end
@test summary(view(a, 1:2)) == "2-element view(::Array{N0f8,1}, 1:2) with eltype Normed{UInt8,8}"
end

@testset "isinteger" begin
# issue #120
for T in generate_fixedpoint_types(:Normed)
T_ints = T.(clamp.(rand(rawtype(T), 500, 500),
ceil(rawtype(T), floattype(T)(typemin(T))),
floor(rawtype(T), floattype(T)(typemax(T)))))
@test all(isinteger.(T_ints))
end
end
Comment on lines +348 to +356
Copy link
Collaborator

Choose a reason for hiding this comment

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

See the comment for Fixed version.


@testset "disambiguation constructors" begin
@test_throws ArgumentError Normed{UInt32,16}('a')
@test_throws InexactError Normed{UInt32,16}(complex(1.0, 1.0))
Expand Down
3 changes: 3 additions & 0 deletions test/runtests.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
using FixedPointNumbers, Test
using FixedPointNumbers: rawtype

@test isempty(detect_ambiguities(FixedPointNumbers, Base, Core))

include("utils.jl")

@testset "normed" begin
include("normed.jl")
end
Expand Down
13 changes: 1 addition & 12 deletions test/traits.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,7 @@
@testset "floattype" begin
function _is_fixed_type(x::Symbol)
try
@eval $(x) isa Type && $(x) <: FixedPoint && return true
catch
return false
end
end

fixed_types = setdiff(filter(_is_fixed_type, names(FixedPointNumbers)), [:Fixed, :Normed, :FixedPoint])
fixed_types = [@eval $(x) for x in fixed_types]

exact_types = vcat([UInt8, UInt16, UInt32, UInt64, UInt128, Bool,
Int8, Int16, Int32, Int64, Int128],
fixed_types)
generate_fixedpoint_types())
for T in exact_types
@test typemax(T) <= maxintfloat(floattype(T))
end
Expand Down
18 changes: 18 additions & 0 deletions test/utils.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
""" return true if symbol `x` is a `ST` type"""
function _is_type(x::Symbol, ST::Symbol)
try
@eval $(x) isa Type && $(x) <: $(ST) && return true
catch
return false
end
end

"""
generate_fixedpoint_types(ST::Symbol=:FixedPoint)

generate a list of concrete `ST` types, where `ST ∈ (:FixedPoint, :Fixed, :Normed)`
"""
function generate_fixedpoint_types(ST::Symbol=:FixedPoint)
fixed_types = setdiff(filter(x->_is_type(x, ST), names(FixedPointNumbers)), [:Fixed, :Normed, :FixedPoint])
fixed_types = [@eval $(x) for x in fixed_types]
end
Comment on lines +1 to +18
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about using isconcretetype(T) instead of setdiff and renaming this function to something like concrete_subtypes.

"""
    concrete_subtypes(T::Type)
generate a list of concrete subtypes of `T` which are defined in `FixedPointNumbers`.
"""
function concrete_subtypes(T::Type)
    is_concrete_subtype(x) = x isa Type && x <: T && isconcretetype(x)
    filter(is_concrete_subtype, eval.(names(FixedPointNumbers)))
end

I believe that any exceptions do not occur in unbroken modules. If any exceptions should occur, they should not be caught.