Type-piracy in similar with OffsetAxis
#87
Comments
|
It's basically "sanctioned type piracy." The issue is that Base is incapable of creating arrays with axes that start at anything other than 1. The dispatch hierarchy for Base ensures that any combination of inputs that can be reduced to axes that start at 1 will get caught by methods in Base. This method is the fallback, in case that doesn't happen. That's OK here because this is the foundational package for arrays with offset axes. Any other package that wants to define offset axes has to define their own axis types. See https://docs.julialang.org/en/latest/devdocs/offset-arrays/#Custom-AbstractUnitRange-types-1. Does that cover it? Or is there an issue that I'm not understanding? |
|
The issue is that as designed now type ambiguities in other packages (in this case BlockArrays.jl) only are triggered if a user loads OffsetArrays.jl. I think a better approach would be in Base to add something like: _offset_similar(_) = error("Not implemented in Base. Please use external package.")
Base.similar(A::AbstractArray, ::Type{T}, inds::Tuple{OffsetAxis,Vararg{OffsetAxis}}) where T = _offset_similar(A, T, index)then OffsetArrays.jl would overload |
|
If you can sync this across Base, BlockArrays, and OffsetArrays, by all means go for it! Might want to also check CatIndices.jl as an example of a package that defines its own index types. |
|
I won’t be able to tackle this right now but perhaps in the future. Maybe just leave this open for now? |
|
This type piracy has become worse with #90: now |
|
Base ranges need an overhaul anyway to address JuliaLang/julia#30950. That PR was starting to turn into a nightmare, I think the best option will be to start fresh and go slowly, thinking very carefully about how to avoid breakage. (There is a chance that those issues simply can't be fixed until Julia 2.0 😦 ). It would be good to address this as part of that effort. I don't see myself as having time for it in the next few weeks or more, so definitely looking for help from others! |
|
I'm working on something similar to this right now (OffsetAxes.jl) that builds on AxisIndices.jl. If I understand correctly (which it's possible I don't) then Would it make sense to have a more explicit argument for what is happening here? Perhaps something like |
|
I think the abmiguity issues caused by this piracy is nearly resolved now. At least, it's resolved for OffsetArrays.jl/src/OffsetArrays.jl Line 219 in f20ecfc and #157 changed OffsetAxisKnownLength to mean Union{Integer, AbstractUnitRange} (which is the same thing as Base.DimOrInd).So packages can define similar for dims::Tuple{DimOrInd, Vararg{DimOrInd}} without triggering ambiguity with this method and without depending on a type internal to OffsetArrays. This signature is almost the current "official" recommendation inhttps://github.com/JuliaLang/julia/blob/110f1256ac88ee0f12885289e807229acf9c44a3/base/abstractarray.jl#L723-L727 (it says UnitRange rather than AbstractUnitRange).Perhaps the official documented recommendation should now be to use Base.DimOrInd when specializing similar for offset arrays.
|
|
I just encountered something similar when trying to This feels a bit naughty to me. |
|
To be fair, that's a MethodError without this package, and the expected answer with it. More surprising is: |
|
Ah, my bad - hadn't thought to try it, assumed the Base one would accept any Integer. Yes - that StackOverflowError is how I found this! My original case that crashed was something like |
|
I assumed the same. And while I didn't write this, if I had, it would have been accidental piracy from this assumption -- trying to allow UnitRanges in otherwise the same Union as Base has. Maybe the signature here should just be tightened to match. (Also, how did you land up with an |
|
Many of the julia> reshape(rand(2,3), (Int32(3), Int32(2)))
3×2 Array{Float64,2}:
0.296956 0.113515
0.0535506 0.564747
0.90234 0.00424624
julia> reshape(rand(2,3), (Int32(3), :))
ERROR: MethodError: no method matching reshape(::Array{Float64,2}, ::Tuple{Int32,Colon})Even stranger is the fact that this is only permitted when the sizes are passed as a julia> reshape(rand(2,3), Int32(3), Int32(2))
ERROR: MethodError: no method matching reshape(::Array{Float64,2}, ::Int32, ::Int32)I wonder if there are a few method signatures that need to be altered here? |
|
I found this funny behaviour, reproducible in a fresh temp project with only OffsetArrays: julia> using OffsetArrays
julia> A = rand(10, 10);
julia> A[Base.Slice(1:10)]
10-element OffsetArray(::Vector{Float64}, 1:10) with eltype Float64 with indices 1:10:
0.31581111033816844
0.2408986878236269
0.865276045142753
0.4158474542637325
0.5674733494230301
0.5910918167329304
0.5583008496658141
0.8863892215050752
0.9094492113020204
0.1511050866350474Without OffsetArrays, it's a method error: julia> A[Base.Slice(1:10)]
ERROR: MethodError: no method matching similar(::Matrix{Float64}, ::Type{Float64}, ::Tuple{Base.IdentityUnitRange{UnitRange{Int64}}}) |
|
I think such type-piracy, where an error becomes a valid operation, isn't considered too serious. After all, julia> A[Base.Slice(9:10)]
2-element OffsetArray(::Vector{Float64}, 9:10) with eltype Float64 with indices 9:10:
0.13029549937036167
0.024726720571425886
julia> UnitRange(axes(A[Base.Slice(1:10)], 1))
1:10
julia> UnitRange(axes(A[Base.Slice(9:10)], 1))
9:10In fact, interestingly, such an operation for ranges does not preserve the axes even with julia> r = 1:10; s = Base.Slice(9:10);
julia> r[s] |> axes
(Base.OneTo(2),)
julia> Vector(r)[s] |> axes
(OffsetArrays.IdOffsetRange(values=9:10, indices=9:10),)
julia> r == Vector(r)
true
julia> r[s] == Vector(r)[s] # should hold, but doesn't as the axes don't match
falseThe reason for this is perhaps historical. I had proposed a fix for this, but I haven't looked at it in a while. |
|
@jishnub I have to disagree that an error not throwing is not serious type piracy. When OffsetArrays.jl is just a secondary dependency not loaded by the user, it can still break the locality of errors when debugging - there are many typos with base julia methods and types that fail in Base but don't with OffsetArrays.jl loaded. That is not something you look for, because it shouldn't be possible. Especially when you havent even intentionally loaded OffsetArrays.jl. If every package did this Julia would be unusable. I'm not sure why OffsetArrays still gets a pass. |
@rafaqz can you give some concrete examples? I'm in agreement with @jishnub, but happy to be persuaded otherwise. I don't understand what kinds of typos you're talking about, so I'd need some hints from you to understand how what you're describing could happen. |
|
The most obvious one from my other issue: I typed something like Hurrah, Then get an error somewhere totally different, with a bounds error. I didnt even load OffsetArrays. It was only installed as a dependency. With the examples above, its easy to pass These mistakes should be quickly uncovered in predictable ways. |
For posterity: this references #306 OK, but FYI I happened to see this and not that. The more explicit you can be the better: "this bugs me" is not really actionable. Anyway, sorry it annoyed you and I see your point. |
|
Absolutely, apologies for not being clearer. Part of the vagueness is how I experience the problems with type piracy on errors is quite abstract. " Essentially I don't consider type piracy on Base methods/types in the list of possible problems when I'm debugging code, because its a large mental overhead to do that. Generally we have consistent set of methods and types in scope we can only intentionally add to by importing packages. And we know:
When we hit errors, we can apply our understanding of these scopes to limit the potential causes of problems, and solve them more quickly. We can look at the stack trace, and know the methods and types that could have led to it, and scan for them in our code. Type piracy on base methods and types breaks these assumptions. The possible scope of the call stack for any method call becomes all of the packages currently loaded and all of their dependency trees (realistically its the packages that are allowed to do type piracy, but I'm not across the history of that). We also can't go in reverse from an error in an OffsetArrays.jl method to looking for a method or type from a package that may depend on OffsetArrays.jl. Instead we have to inspect every line of our code as a potential source of the error. To me, getting errors in predictable ways is valuable. OffsetArrays doing type piracy isn't just about occasionally getting these strange errors, but about breaking an efficient approach to programming that ignores the possibility of these problems completely. |
|
I apologize for not having worked through this long thread. I only want to point out that there is also a conflict with JuliaArrays/FFTViews.jl: If there you add |
|
This is a common problem with OffsetArrays and tests. I've hit it in both DimensionalData.jl and GeometryOps.jl. The worse case is the opposite one, where tests pass when OffsetArrays is loaded but break when its not. |
|
Cross-referencing #306 (comment), which might be a way to avoid this for packages. |
|
In light of recently posted #369 I will add my two cents to the discussion here. Below my opinion on the matter:
|
This is type-piracy:
OffsetArrays.jl/src/OffsetArrays.jl
Line 99 in d50463f
The text was updated successfully, but these errors were encountered: