Make InfExtendedReal a bitstype #13
Conversation
Codecov Report
@@ Coverage Diff @@
## master #13 +/- ##
============================================
+ Coverage 67.40% 100.00% +32.59%
============================================
Files 18 18
Lines 181 189 +8
============================================
+ Hits 122 189 +67
+ Misses 59 0 -59
Continue to review full report at Codecov.
|
| @@ -1,6 +1,6 @@ | |||
| Base.isfinite(x::InfExtendedReal) = isfinite(x.val) | |||
| Base.isfinite(x::InfExtendedReal) = x.flag == FINITE && isfinite(x.finitevalue) | |||
There was a problem hiding this comment.
Seems strange to be storing non-finite values in a field called finitevalue
There was a problem hiding this comment.
Normally x.finitevalue will only ever be finite, unless the user does something like InfExtendedReal{Float64}(Inf).
There was a problem hiding this comment.
Yeah I did the same thing in my dates PR as well. I didn't necessarily think it made sense to also check for finite in x.finitevalue. Guess it makes sense to remove it.
src/infextendedreal/io.jl
Outdated
| @@ -1 +1,10 @@ | |||
| Base.show(io::IO, x::InfExtendedReal) = show(io, x.val) | |||
| function Base.show(io::IO, x::T) where {T<:InfExtendedReal} | |||
| value = isposinf(x) ? ∞ : isneginf(x) ? -∞ : x.finitevalue | |||
There was a problem hiding this comment.
| value = isposinf(x) ? ∞ : isneginf(x) ? -∞ : x.finitevalue | |
| value = x.val |
src/infextendedreal/base.jl
Outdated
| """ | ||
| Since InfExtendedReal is a subtype of Real, and Infinite is also a subtype of real, | ||
| we can just use `x.val` to get either the finite value, or the infinite value. This will | ||
| make arithmetic much simpler. | ||
| """ |
There was a problem hiding this comment.
Can you make this a comment? Else it pollutes the documentation for getproperty.
|
The inferred tests are failing because the 2-argument form of |
src/infextendedreal/comparison.jl
Outdated
| @@ -1,4 +1,4 @@ | |||
| Base.isfinite(x::InfExtendedReal) = x.flag == FINITE && isfinite(x.finitevalue) | |||
There was a problem hiding this comment.
Change this back please. My previous comment was supposed to mean: normally x.finitevalue will be finite (and for things like Int this check can be optimised out anyway) but it's good to check in case we actually have InfExtendedReal{Float64}(Inf).
There was a problem hiding this comment.
Ah yes that's true. If you define it with the type specifically it will still be an InfExtendedReal rather than a plain Float64.
Edit: Also the page didn't update with your comment above when I was reading comments, so I completely missed it. Sorry!
There was a problem hiding this comment.
That being said, would it make sense to modify the constructor to change InfExtendedReal{Float64}(Inf) to InfExtendedReal{Float64}(∞)?
There was a problem hiding this comment.
Perhaps, I was wondering the same thing myself, but it's a separate issue.
There was a problem hiding this comment.
I noticed that this constructor exists InfExtendedReal{T}(x::T) where {T<:Real} = new(FINITE, x) and also this function exists InfExtendedReal{T}(x::Real) where {T<:Real} = InfExtendedReal{T}(isinf(x) ? convert(Infinite, x) : convert(T, x)).
I can't seem to figure out under what circumstances the second function would ever trigger. I'm not sure it does
There was a problem hiding this comment.
Oh I see, when T is Int it would trigger but not if T is say Float64
|
Opened this PR to add the 2 arg @inferred to Compat.jl JuliaLang/Compat.jl#706 |
|
Ok, I think this should be about ready to merge if anybody wants to give it a final check. |
|
Thanks again! I'll release this as v0.2 soon. |
|
Oh and thanks for the extensive tests. |
|
@cjdoris I'd like to do one more PR to clean everything up and consolidate things. Let me get that going, and when thats done we can release the v0.2 if thats ok with you |
|
ah ok no problemo. :) I'll throw an issue up and get it going |
closes #4
So here's an attempt at making InfExtendedReal a bitstype. Since a lot of the arithmetic and whatnot assumes it's working with a
Realvalue, I added ax.valproperty accessor that will return the finite value or -+infinity. This seemed to be the easiest way to getInfExtendedRealinto a bitstype without completely rewriting all the code for it.That being said, I'm a little unsure how to fix the test changes where the @inferred tests were failing. I added the types that they should expect, but I'm not sure if thats the correct solution or if there is a better one.
Aside from that there were a couple of other minor changes to get the
x.valaccessor working properly. I might want to add more tests as well as I'm not sure everything is covered with the current set.