SciML / Static.jl

Static types useful for dispatch and generated functions.
MIT License
55 stars 15 forks source link

Making StaticInt an Integer again? #73

Open oschulz opened 2 years ago

oschulz commented 2 years ago

Since Static v0.7, StaticInt is not a subtype of Integer anymore. Would it be possible to change that again? Many constructions that used StaticInt in places that require an Integer just aren't possible anymore now. For example, Base.OneTo(static(1)) doesn't work anymore and I'm sure there are lots of other uses cases that will find it hard (or impossible) to adapt their code to v0.7).

CC @cscherrer

chriselrod commented 2 years ago

Base.oneto(static(N)) should return static(1):static(N). I'd be fine with having Base.OneTo also do this, but some people dislike having "constructors" return objects of different types.

The issue with making StaticInt an integer is that it causes a tremendous amount of invalidations.

cscherrer commented 2 years ago

Is there any update on the suggestion to get StaticInt into Base? This seems like the obvious solution. There are concrete problems that arise from not doing this, are there any that would be caused by doing it? Is there somewhere we can help argue the case?

Also... Are there also invalidations caused by making StaticFloat64 <: AbstractFloat?

oschulz commented 2 years ago

Base.oneto(static(N)) should return static(1):static(N).

We can add that, right?

Tokazama commented 2 years ago

That's a bit tricky because we'd have to add the optionally static range types and then we are moving into ArrayInterface territory. The separation is far from perfect but there are certain components here that just don't need to be in ArrayInterface

Tokazama commented 2 years ago

Can we change it back to to a subtype of Integer, yes. However, it would be difficult to accomplish because it causes a ridiculous amount of invalidations. This varies between Julia releases because there's on ongoing effort to reduce ambiguities that cause invalidations in base; but integer is used so widely that they pop up unexpectedly all the time.

Tokazama commented 2 years ago

Here's the issue you can follow to voice opinions on its addition to base https://github.com/JuliaLang/julia/pull/44538#issuecomment-1164751412.

oschulz commented 2 years ago

Thanks for the detailed explanation @Tokazama - large numbers of invalidations are indeed definitely not great.

@cscherrer , do you think we can use something like static(1):static(n) directly?

Tokazama commented 2 years ago

I'm open to PRs and suggestions, but I went through at least 3 rewrites of this package trying to find a solution that worked at all. At one point I started speaking in tongues (well, binary). So be careful going into any rewrites for your own sanity's well being

oschulz commented 2 years ago

Oh, sorry - I meant work around StaticInt not being an Integer anymore in downsteam packages.

Tokazama commented 2 years ago

@cscherrer, if you're interested in separating out StataticFloat64 as a subtype of AbstractFloat I'd be willing to review a PR. I would caution you to look out for how much that proportionally increases load time though.

oschulz commented 2 years ago

I've been trying to find a range type that works with the new static - static(1):static(5) doesn't work, the only thing that seems to work is static(1):static(1):static(5), which is really cumbersome and hard to fit into generic code that naturally would deal with UnitRanges.

Do the invalidations of "static being an integer" actually affect Static.jl itself or packages that use it?

Tokazama commented 2 years ago

IIRC invalidations explode when we define conversion to Int.

Tokazama commented 2 years ago

Here's the big one we get when making a new subtype of Integer and converting it to Int

 inserting (::Type{T})(x::StaticInts.StaticInt) where T<:Integer in StaticInts at /Users/zchristensen/projects/Static.jl/libs/StaticInts/src/StaticInts.jl:27 invalidated:
   mt_backedges:  1: signature Tuple{Type{Int64}, Integer} triggered MethodInstance for convert(::Type{Int64}, ::Integer) (0 children)
                  2: signature Tuple{Type{UInt64}, Integer} triggered MethodInstance for convert(::Type{UInt64}, ::Integer) (0 children)
                  3: signature Tuple{Type{UInt32}, Integer} triggered MethodInstance for VersionNumber(::Integer, ::Int64, ::Int64, ::Tuple{}, ::Tuple{}) (1 children)
                  4: signature Tuple{Type{UInt64}, Integer} triggered MethodInstance for UInt64(::Enum) (1 children)
                  5: signature Tuple{Type{Int64}, Integer} triggered MethodInstance for Int64(::Enum) (1 children)
                  6: signature Tuple{Type{Int64}, Integer} triggered MethodInstance for insert!(::BitVector, ::Integer, ::Any) (2 children)
                  7: signature Tuple{Type{Int64}, Integer} triggered MethodInstance for findprev(::InteractiveUtils.var"#34#39", ::AbstractString, ::Integer) (3 children)
                  8: signature Tuple{Type{Int64}, Integer} triggered MethodInstance for Base._array_for(::Type{Union{Int64, Symbol}}, ::Base.HasLength, ::Integer) (6 children)
                  9: signature Tuple{Type{Int64}, Integer} triggered MethodInstance for Base._array_for(::Type{Base.PkgId}, ::Base.HasLength, ::Integer) (6 children)
                 10: signature Tuple{Type{Int64}, Integer} triggered MethodInstance for findnext(::Pkg.Types.var"#27#28"{String}, ::AbstractString, ::Integer) (38 children)
                 11: signature Tuple{Type{Int64}, Integer} triggered MethodInstance for Base.to_shape(::Integer) (470 children)
Tokazama commented 2 years ago

This is all you need to reproduce

module StaticInts

struct StaticInt{N} <: Integer
    StaticInt{N}() where {N} = new{N::Int}()
end

Base.Int(::StaticInt{N}) where {N} = N

end
oschulz commented 1 year ago

@Tokazama , following up on the discussion on #87, I tried just changing StaticInteger{N} <: Number to StaticInteger{N} <: Integer as in initial test and counted invalidations via

using SnoopCompileCore
invalidations = @snoopr begin
    using Static
end
using SnoopCompile 
length(uinvalidated(invalidations))

and load time with @time_imports (20 measurements each time).

Given that, could we pursue making StaticInteger and Integer (which will re-enable statically sized Fill arrays and so on), or is that number of invalidations unacceptable?

Would StaticInteger{N} <: Integer allow us to remove OptionallyStaticUnitRange and OptionallyStaticStepRange? That might bring invalidations down again:

chriselrod commented 1 year ago

Would StaticInteger{N} <: Integer allow us to remove OptionallyStaticUnitRange and OptionallyStaticStepRange?

No. I suggest looking at the invalidations and PRing to remove the type instabilities that let them happen.

Tokazama commented 1 year ago

IRRC, a lot of the current invalidations are due to things with StaticBool and have improved on Julia's master branch.

oschulz commented 1 year ago

Ok so there no hard blockers? Then I'd try my hand at a StaticInteger{N} <: Integer PR.

Tokazama commented 1 year ago

I think the range invalidation s are mostly due to similar