fix isinteger by johnnychen94 · Pull Request #123 · JuliaMath/FixedPointNumbers.jl

johnnychen94 · 2019-05-15T07:38:33Z

fixes #120 : isinteger(1N0f8) should be true

P.S. I don't quite understand the magic behind bits operation, but the tests pass with this patch.

fixes JuliaMath#120 : `isinteger(1N0f8)` should be true

codecov-io · 2019-05-15T07:54:18Z

Codecov Report

Merging #123 into master will increase coverage by 0.1%.
The diff coverage is 100%.

@@           Coverage Diff            @@
##           master    #123     +/-   ##
========================================
+ Coverage    78.3%   78.4%   +0.1%     
========================================
  Files           3       3             
  Lines         212     213      +1     
========================================
+ Hits          166     167      +1     
  Misses         46      46

Impacted Files	Coverage Δ
src/normed.jl	`80.64% <100%> (+0.21%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5729a89...5cf20d5. Read the comment docs.

test/fixed.jl

There's no Integer between 0 and 1/2, so these symbols are meaningless.

This helps improve test type coverage

johnnychen94 · 2019-05-16T03:54:51Z

After solving the conflicts, I failed to rebase the commits (not a git expert). You could either squash it or rebase it into two commits (if it's possible):

make generate_fixedpoint_types a test utils
fix isinteger

johnnychen94 · 2019-05-16T05:38:25Z

Wow, this patch fails in Windows x86. That's really out of my ability then. Can someone take it over to let the test pass?

kimikage

@johnnychen94, please replace the 1<<f-1 with one(T)<<f-0x1.
(Don't forget to restore the CI settings.:smile:)

Edit:
I'm sorry, but the suggestions were wrong. Now fixed.

kimikage · 2019-10-11T23:52:16Z

test/utils.jl

+""" 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


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.

kimikage · 2019-10-12T03:35:04Z

src/normed.jl

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


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"

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

kimikage · 2019-10-12T08:11:55Z

test/fixed.jl

+@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


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.

kimikage · 2019-10-12T08:29:34Z

test/normed.jl

+@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


See the comment for Fixed version.

johnnychen94 · 2019-10-12T10:31:18Z

Thank you for putting your time investigating it! I'll find time to fix them accordingly. (I'm a little busy these days, and my night time is spent on ReferenceTests.jl, so if you can't wait, you can continue the work here :D)

kimikage · 2019-10-12T11:26:32Z

As I did not care about this issue until yesterday, please take your time.

kimikage · 2019-11-22T12:42:10Z

I am sorry for hindering the rebase, but now we can use isinteger(x) == isinteger(float(x)) to test Normed{UInt8}, Normed{UInt16} and Fixed.
However, float(x::Normed{UInt32}) and float(x::Normed{UInt64}) still have 1-lsb errors. If you want to introduce the random testing for them, extra checks are required.
IMO, the implementation of isinteger should be so primitive that only the numbers around the boundaries which might possibly cause overflow/underflow should be checked.

kimikage · 2019-12-24T03:10:08Z

I'm ready to fix isinteger with minimal tests (i.e. without random tests).
kimikage/FixedPointNumbers.jl@commonize1...kimikage:isinteger

Since the master has left this branch, I'm going to submit a new PR.

johnnychen94 · 2019-12-24T04:50:09Z

@kimikage Please feel free to do so 👍 , recently I just spent too much time here and I'm trying to catch up with my daily research projects in ECNU.

fix isinteger

5cf20d5

fixes JuliaMath#120 : `isinteger(1N0f8)` should be true

timholy reviewed May 15, 2019

View reviewed changes

test/fixed.jl Outdated Show resolved Hide resolved

johnnychen94 added 7 commits May 15, 2019 20:29

temp fix

067dc1f

Remove N*f0 symbols (JuliaMath#121)

4bcc60c

There's no Integer between 0 and 1/2, so these symbols are meaningless.

enhance and export floattype (JuliaMath#122)

279ea8a

make generate_fixedpoint_types a test utils

0d72abf

This helps improve test type coverage

improve coverage of isinteger

8b19b8d

Merge branch 'master' into isinteger

7ee8a04

include utils.jl

e9b2da1

johnnychen94 added 2 commits May 16, 2019 13:50

change BigInt to rawtype

561eb94

disable unrelated CI

d9a1201

johnnychen94 force-pushed the isinteger branch from db9bcb0 to d9a1201 Compare May 16, 2019 05:55

johnnychen94 mentioned this pull request May 19, 2019

bump patch release #124

Merged

kimikage mentioned this pull request Oct 12, 2019

Rounding errors in Normed to Float conversions #129

Closed

kimikage requested changes Oct 12, 2019

View reviewed changes

kimikage mentioned this pull request Oct 31, 2019

Improve accuracy of conversions from floating-point numbers (Fixes #102) #131

Merged

kimikage mentioned this pull request Dec 23, 2019

Commonize code between Fixed and Normed #151

Merged

johnnychen94 closed this Dec 24, 2019

kimikage mentioned this pull request Jan 12, 2020

Fix isinteger #165

Merged

johnnychen94 deleted the isinteger branch January 12, 2020 17:39

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

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

Conversation

johnnychen94 commented May 15, 2019

Uh oh!

codecov-io commented May 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

johnnychen94 commented May 16, 2019

Uh oh!

johnnychen94 commented May 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kimikage left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kimikage Oct 11, 2019

Choose a reason for hiding this comment

Uh oh!

kimikage Oct 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kimikage Oct 14, 2019

Choose a reason for hiding this comment

Uh oh!

kimikage Oct 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kimikage Oct 12, 2019

Choose a reason for hiding this comment

Uh oh!

johnnychen94 commented Oct 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kimikage commented Oct 12, 2019

Uh oh!

kimikage commented Nov 22, 2019

Uh oh!

kimikage commented Dec 24, 2019

Uh oh!

johnnychen94 commented Dec 24, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-io commented May 15, 2019 •

edited

Loading

johnnychen94 commented May 16, 2019 •

edited

Loading

kimikage left a comment •

edited

Loading

kimikage Oct 12, 2019 •

edited

Loading

kimikage Oct 12, 2019 •

edited

Loading

johnnychen94 commented Oct 12, 2019 •

edited

Loading