MilesCranmer / DispatchDoctor.jl

The dispatch doctor prescribes type stability
Apache License 2.0
128 stars 6 forks source link

Implement mechanism for ignoring unions #31

Closed MilesCranmer closed 3 weeks ago

MilesCranmer commented 4 weeks ago

@ChrisRackauckas is this what you are looking for?

For example:

@stable default_ignore_union = true begin
    # Just a union:
    f(x) = x > 0 ? x : 0.0
    # Full-blown type instability:
    g() = Val(rand())
end

f(0) # No error; just a union
@test_throws TypeInstabilityError g()

This doesn't affect any existing code as ignore_union is set to true by default. As with other parameters you can configure this with Preferences.jl – instability_check_ignore_union is the new parameter here.

As usual this should be removed by the compiler if the code is stable.

Fixes #29. The PR also moves the options parsing to a new file parse_options.jl.

coveralls commented 4 weeks ago

Coverage Status

coverage: 99.678% (-0.3%) from 100.0% when pulling 101d2ae8b951c3caa3171e0c22980a36914826f4 on ignore-union-split into 02b46f6060c84c632dedf5e602dd3ca6a2c72d95 on main.

ChrisRackauckas commented 4 weeks ago

It should only catch unions up to the union splitting size of the given module. It seems like this blanket opts all unions out?

MilesCranmer commented 4 weeks ago

How should I get the union splitting size?

MilesCranmer commented 4 weeks ago

The only thing I could find on this is Keno's comment on discourse:

Yes it works with more than two types, but there’s various limits in various parts of the compiler that’ll kick in if you go much beyond that (limits to the number of union elements, number of applicable methods considered, maximum union splitting branching factor, etc.). As a rule of thumb, I’d say around 3 or 4 union elements will probably always stay under all these limits, but it depends on the specific case and which part you care about (e.g. the storage optimization is effective up to 255 elements I believe).

So are you thinking this could be another parameter the user would set? I guess it could be unified under a single parameter with the default being 0 (meaning: no unions allowed).

MilesCranmer commented 4 weeks ago

@ChrisRackauckas here is the new API (which is what I think you mean)

# Breaks with 4-part union:
@stable default_union_limit=3 begin
    function f(x)
        x = Union{Float16,Float32,Float64,BigFloat}[
            one(Float16), one(Float32), one(Float64), one(BigFloat)
        ]
        return x[rand(1:4)]
    end
end

@test_throws TypeInstabilityError f(0)

# But, now it works:
@stable default_union_limit=4 begin
    function g(x)
        x = Union{Float16,Float32,Float64,BigFloat}[
            one(Float16), one(Float32), one(Float64), one(BigFloat)
        ]
        return x[rand(1:4)]
    end
end

@test g(0) == 1

Sound good?

As before you can override the default union limit locally (single package) or globally with Preferences.jl. In this case with "instability_check_union_limit".

MilesCranmer commented 3 weeks ago

Ping, let me know if you are happy with this and I can merge

ChrisRackauckas commented 3 weeks ago

It's good enough. I think a more sophisticated way should follow but it may not be doable with the current architecture.