Revert missing to nothings #245

ChrisRackauckas · 2022-03-01T00:38:04Z

#232 was found to be a huge mistake. For interface checks, you want nothing instead of missing because you want to handle the cases. The issue with missing is that it propagates, missing + 1 == missing. We don't want that here, we want clear and precise error messages at the spot where the nothing is found if it's not supposed to be there and it's supposed to be handled. That is antithetical to most of its usage here. So this is a strong revert with a breaking update, which should fix downstream libraries (LoopVectorization, OrdinaryDiffEq, etc.) which were never received the proper downstream PRs from the original change anyways.

#232 was found to be a huge mistake. For interface checks, you want `nothing` instead of `missing` because you want to handle the cases. The issue with `missing` is that it propagates, `missing + 1 == missing`. We don't want that here, we want clear and precise error messages at the spot where the `nothing` is found if it's not supposed to be there and it's supposed to be handled. That is antithetical to most of its usage here. So this is a strong revert with a breaking update, which should fix downstream libraries (LoopVectorization, OrdinaryDiffEq, etc.) which were never received the proper downstream PRs from the original change anyways.

ChrisRackauckas · 2022-03-01T00:38:21Z

@YingboMa

YingboMa · 2022-03-01T00:49:59Z

The test failure seems real.

chriselrod

For interface checks, you want nothing instead of missing because you want to handle the cases.

I'd just like to add that other languages like Rust and C++17 have some form of Optionals for this reason.
There are Julia packages for this too, but Union{T,Nothing} does most of the same thing. It's doesn't force you to manually extract the T, which would make it a little more obvious when you don't do it, but both still throw an error when you try to use it incorrectly (one with an explicit variation of "empty optional" error, the other with a method error).

I think this sort of explicitness makes larger codebases easier to maintain and read.
Maintenance ease is due to error messages closer to the source, as well as clearer error messages.

I also view these known_ methods as more for package development / trying to write optimized library code than I do for their use in throw-away scripts, where being slick and ease of writing lots of functioning code are a priority.

chriselrod · 2022-03-01T00:54:30Z

Of course, my actual approval is conditional on tests passing.

ChrisRackauckas · 2022-03-01T12:23:41Z

@chriselrod should ArrayInterface.OptionallyStaticUnitRange{Int,Int}(1:10) just error?

chriselrod · 2022-03-01T12:44:04Z

@chriselrod should ArrayInterface.OptionallyStaticUnitRange{Int,Int}(1:10) just error?

No, it should return an ArrayInterface.OptionallyStaticUnitRange{Int,Int} that iterates from 1 to 10.

ChrisRackauckas · 2022-03-01T12:46:19Z

but the 1 and the 10 aren't static?

chriselrod · 2022-03-01T12:56:27Z

but the 1 and the 10 aren't static?

It's an OptionallyStatic unit range.

ChrisRackauckas · 2022-03-01T15:46:23Z

MWE:

struct Wrapper{T,N,P<:AbstractArray{T,N}} <: ArrayInterface.AbstractArray2{T,N}
    parent::P
end
ArrayInterface.parent_type(::Type{<:Wrapper{T,N,P}}) where {T,N,P} = P
Base.parent(x::Wrapper) = x.parent
ArrayInterface.device(::Type{T}) where {T<:Wrapper} = ArrayInterface.device(parent_type(T))

x = zeros(100);
A = Wrapper(reshape(view(x, 1:60), (3,4,5)));
B = A .== 0;

MethodError: no method matching *(::Nothing)
Closest candidates are:
  *(::Any, !Matched::Any, !Matched::Any, !Matched::Any...) at C:\Users\accou\AppData\Local\Programs\Julia-1.7.0\share\julia\base\operators.jl:655
  *(!Matched::T, !Matched::T) where T<:Union{Int128, UInt128} at C:\Users\accou\AppData\Local\Programs\Julia-1.7.0\share\julia\base\int.jl:967
  *(!Matched::T, !Matched::T) where T<:Union{Int128, Int16, Int32, Int64, Int8, UInt128, UInt16, UInt32, UInt64, UInt8} at C:\Users\accou\AppData\Local\Programs\Julia-1.7.0\share\julia\base\int.jl:88
  ...
prod(x::Tuple{Nothing}) at tuple.jl:499
_maybe_known_length(#unused#::Base.HasShape{1}, #unused#::Type{Base.OneTo{Int64}}) at ArrayInterface.jl:96
known_length(#unused#::Type{Base.OneTo{Int64}}) at ArrayInterface.jl:95
known_length(x::Base.OneTo{Int64}) at ArrayInterface.jl:89
maybe_static at int.jl:121 [inlined]
static_length at ArrayInterface.jl:38 [inlined]
map at tuple.jl:223 [inlined]
_maybe_size(#unused#::Base.HasShape{3}, a::Wrapper{Float64, 3, Base.ReshapedArray{Float64, 3, SubArray{Float64, 1, Vector{Float64}, Tuple{UnitRange{Int64}}, true}, Tuple{}}}) at size.jl:20
size(a::Wrapper{Float64, 3, Base.ReshapedArray{Float64, 3, SubArray{Float64, 1, Vector{Float64}, Tuple{UnitRange{Int64}}, true}, Tuple{}}}) at size.jl:19
_to_linear at indexing.jl:6 [inlined]
unsafe_getindex(::Wrapper{Float64, 3, Base.ReshapedArray{Float64, 3, SubArray{Float64, 1, Vector{Float64}, Tuple{UnitRange{Int64}}, true}, Tuple{}}}, ::Int64, ::Int64, ::Vararg{Int64}) at indexing.jl:335
getindex(A::Wrapper{Float64, 3, Base.ReshapedArray{Float64, 3, SubArray{Float64, 1, Vector{Float64}, Tuple{UnitRange{Int64}}, true}, Tuple{}}}, args::CartesianIndex{3}) at indexing.jl:308
getindex at ArrayInterface.jl:607 [inlined]
_broadcast_getindex at broadcast.jl:636 [inlined]
_getindex at broadcast.jl:666 [inlined]
_broadcast_getindex at broadcast.jl:642 [inlined]
getindex at broadcast.jl:597 [inlined]
macro expansion at broadcast.jl:961 [inlined]
macro expansion at simdloop.jl:77 [inlined]
copyto! at broadcast.jl:960 [inlined]
copyto! at broadcast.jl:971 [inlined]
copyto! at broadcast.jl:913 [inlined]
copy at broadcast.jl:885 [inlined]
materialize(bc::Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{3}, Nothing, typeof(==), Tuple{Wrapper{Float64, 3, Base.ReshapedArray{Float64, 3, SubArray{Float64, 1, Vector{Float64}, Tuple{UnitRange{Int64}}, true}, Tuple{}}}, Int64}}) at broadcast.jl:860
top-level scope at test.jl:15
eval at boot.jl:373 [inlined]

ChrisRackauckas · 2022-03-01T15:53:04Z

It turns out that AbstractArray2 was just pretty broken.

size(a::A) where {A} = _maybe_size(Base.IteratorSize(A), a)
_maybe_size(::Base.HasShape{N}, a::A) where {N,A} = map(static_length, axes(a))
known_length(x) = known_length(typeof(x))
known_length(::Type{T}) where {T} = _maybe_known_length(Base.IteratorSize(T), T)

which Base.IteratorSize(T) gives Base.OneTo{Int64} and so it's just nothing. So this was just throwing missings the whole time, making it seem like it worked, but in actuality it was kind of just silently failing all over.

ChrisRackauckas · 2022-03-01T15:54:23Z

@chriselrod what should I do about that?

chriselrod · 2022-03-01T20:08:46Z

Tests pass locally, but it breaks the range code. I'll try and undo that.

codecov · 2022-03-01T20:12:45Z

Codecov Report

Merging #245 (d497de8) into master (63617dd) will increase coverage by 0.07%.
The diff coverage is 84.52%.

@@            Coverage Diff             @@
##           master     #245      +/-   ##
==========================================
+ Coverage   89.23%   89.31%   +0.07%     
==========================================
  Files          11       11              
  Lines        1728     1740      +12     
==========================================
+ Hits         1542     1554      +12     
  Misses        186      186

Impacted Files	Coverage Δ
src/indexing.jl	`85.57% <ø> (ø)`
src/size.jl	`87.87% <0.00%> (ø)`
src/stridelayout.jl	`87.18% <74.41%> (ø)`
src/ranges.jl	`92.52% <90.90%> (ø)`
src/ArrayInterface.jl	`90.24% <100.00%> (+0.22%)`	⬆️
src/axes.jl	`92.81% <100.00%> (ø)`
src/dimensions.jl	`97.88% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 63617dd...d497de8. Read the comment docs.

chriselrod · 2022-03-01T20:14:58Z

I made local changes because of type inference failures. Turns out I get those on ArrayInterface 4 (on Julia master) anyway, so I'll revert.
The easiest way to fix type inference problems was to make the dimnames API consistent (nothing indicating a lack of dimname), but that's a breakage and I'd rather not fix the downstream packages.

test/runtests.jl

ChrisRackauckas requested a review from chriselrod March 1, 2022 00:38

chriselrod approved these changes Mar 1, 2022

View reviewed changes

rafaqz mentioned this pull request Mar 1, 2022

New (breaking) Traits based release JuliaGeo/GeoInterface.jl#33

Merged

12 tasks

ChrisRackauckas and others added 5 commits March 1, 2022 09:59

Update Project.toml

f0bc637

Update Project.toml

ed14159

Update Project.toml

808f547

Update Project.toml

c02efe9

only maybe use known_size in _maybe_known_length

af97a8c

Tests pass

f9b16dc

Revert type inference fixes

b020ccb

chriselrod force-pushed the nothing branch from 208b620 to b020ccb Compare March 1, 2022 20:20

chriselrod added 2 commits March 1, 2022 15:41

remove @boundschecks

f64135a

skip failed inference test

6148a9f

chriselrod reviewed Mar 1, 2022

View reviewed changes

test/runtests.jl Outdated Show resolved Hide resolved

coax inference

d497de8

ChrisRackauckas merged commit 7b3cf6b into master Mar 1, 2022

ChrisRackauckas deleted the nothing branch March 1, 2022 22:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revert missing to nothings #245

Revert missing to nothings #245

ChrisRackauckas commented Mar 1, 2022

ChrisRackauckas commented Mar 1, 2022

YingboMa commented Mar 1, 2022

chriselrod left a comment

chriselrod commented Mar 1, 2022

ChrisRackauckas commented Mar 1, 2022

chriselrod commented Mar 1, 2022

ChrisRackauckas commented Mar 1, 2022

chriselrod commented Mar 1, 2022

ChrisRackauckas commented Mar 1, 2022

ChrisRackauckas commented Mar 1, 2022

ChrisRackauckas commented Mar 1, 2022

chriselrod commented Mar 1, 2022 •

edited

Loading

codecov bot commented Mar 1, 2022 •

edited

Loading

chriselrod commented Mar 1, 2022 •

edited

Loading

Revert missing to nothings #245

Revert missing to nothings #245

Conversation

ChrisRackauckas commented Mar 1, 2022

ChrisRackauckas commented Mar 1, 2022

YingboMa commented Mar 1, 2022

chriselrod left a comment

Choose a reason for hiding this comment

chriselrod commented Mar 1, 2022

ChrisRackauckas commented Mar 1, 2022

chriselrod commented Mar 1, 2022

ChrisRackauckas commented Mar 1, 2022

chriselrod commented Mar 1, 2022

ChrisRackauckas commented Mar 1, 2022

ChrisRackauckas commented Mar 1, 2022

ChrisRackauckas commented Mar 1, 2022

chriselrod commented Mar 1, 2022 • edited Loading

codecov bot commented Mar 1, 2022 • edited Loading

Codecov Report

chriselrod commented Mar 1, 2022 • edited Loading

chriselrod commented Mar 1, 2022 •

edited

Loading

codecov bot commented Mar 1, 2022 •

edited

Loading

chriselrod commented Mar 1, 2022 •

edited

Loading