Skip to content
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.

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

InfExtended is not a bitstype #4

Closed
omus opened this issue Jun 1, 2020 · 9 comments · Fixed by #13
Closed

InfExtended is not a bitstype #4

omus opened this issue Jun 1, 2020 · 9 comments · Fixed by #13
Labels
enhancement New feature or request

Comments

@omus
Copy link
Contributor

omus commented Jun 1, 2020

It would be great if InfExtended was a bitstype so it could be used as a value in a parametric struct:

julia> using Infinity

julia> struct Foo{T} end

julia> Foo{∞}
Foo{∞}

julia> Foo{InfExtended{Int64}(∞)}
ERROR: TypeError: in Type, in parameter, expected Int64, got InfExtended{Int64}
Stacktrace:
 [1] top-level scope at REPL[6]:1

julia> isbitstype(InfExtended{Int64})
false
@omus
Copy link
Contributor Author

omus commented Jun 1, 2020

Something like this would allow the extended type to be a bitstype:

using Infinity

@enum InfinityEnum::UInt8 finite neginf posinf

struct InfExtended2{T<:Real} <: Real
    val::T
    inf::InfinityEnum

    InfExtended2{T}(x::T) where {T<:Real} = new(x, finite)
    function InfExtended2{T}(x::Infinite) where {T<:Real}
        if x == PosInf
            new(x, posinf)
        else
            new(x, neginf)
        end
    end
end
julia> isbitstype(InfExtended2{Int64})
true

@cjdoris
Copy link
Owner

cjdoris commented Jun 2, 2020

Yes, good idea --- the current implementation using Union{T,Infinity} was merely convenience. It's a shame that unions of bits types are not themselves bits types.

I don't have time at the moment to do this, but feel free to send a PR. Let me know if you're planning to do this.

If so, I think I like the following naming conventions. Also I think there was a bug in your inner constructor in the infinite case. This should work:

@enum InfFlag::UInt8 finite neginf posinf

struct InfExtended{T<:Real} <: Real
    flag :: InfFlag
    finitevalue :: T

    InfExtended{T}(x::T) where {T<:Real} = new{T}(isfinite, x)
    InfExtended{T}(x::Infinite) where {T<:Real} = new{T}(x==PosInf ? isposinf : isneginf)
end

@cjdoris cjdoris added the enhancement New feature or request label Jun 2, 2020
@cjdoris
Copy link
Owner

cjdoris commented Jun 2, 2020

I have just added InfFlag, because it was going to be needed for #3.

@fchorney
Copy link
Contributor

fchorney commented Jun 3, 2020

Would the implementation be more so like:

@enum InfFlag::UInt8 finite neginf posinf

struct InfExtended{T<:Real} <: Real
    flag :: InfFlag
    finitevalue :: T

    InfExtended{T}(x::T) where {T<:Real} = new{T}(finite, x)
    InfExtended{T}(x::Infinite) where {T<:Real} = new{T}(x==PosInf ? posinf : neginf)
end

But also, wouldn't the second constructor end up making finitevalue empty or garbage data?

@omus
Copy link
Contributor Author

omus commented Jun 3, 2020

But also, wouldn't the second constructor end up making finitevalue empty or garbage data?

@fchorney your example looks correct. You are also correct in that the second constructor just has garbage for the finitevalue when the flag is non-finite. We could alternatively use zero(T) but that is slightly slower really the value in finitevalue doesn't matter.

@fchorney
Copy link
Contributor

fchorney commented Jun 3, 2020

ok that's kind of what I thought, since if the flag is infinite of any kind than we don't ever care about the value.

@cjdoris
Copy link
Owner

cjdoris commented Jun 6, 2020

ok that's kind of what I thought, since if the flag is infinite of any kind than we don't ever care about the value.

Indeed. For example DataValues.jl uses basically the same construction: https://github.com/queryverse/DataValues.jl/blob/103f4b3b4c3c2a38958aad8bb156d8af0e37e074/src/scalar/core.jl#L1-L7

@cjdoris
Copy link
Owner

cjdoris commented Jun 6, 2020

@omus Are you going to work on this?

@omus
Copy link
Contributor Author

omus commented Jun 9, 2020

I'm not working on this currently

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants