OffsetArrays.jl
OffsetArrays.jl copied to clipboard
Clarify `==`and `===` for `axes` tests
In the tests I see that there are some places where ===
is used to compare results of axes
, which is good since 1:3 == Base.OneTo(3)
, and some places want to test specifically for Base.OneTo(3)
But I also see many cases of e.g. a == IdOffsetRange(3:5)
and a == IdentityUnitRange(-4:-3)
, and I don't see the point of doing that instead of a == 3:5
, unless the test is really intended to check for ===
, in which case in at least a few places it should be a === OffsetArrays.IdOffsetRange(Base.OneTo(3), 2)
.
For example: https://github.com/JuliaArrays/OffsetArrays.jl/blob/7294f3dcab6e6117b15eea3b98ce2c2175d08194/test/runtests.jl#L446
Making the tests too specific will cause them to fail when they maybe should not on changes like https://github.com/JuliaArrays/OffsetArrays.jl/pull/119
On the other hand, having specific tests are a way to document behavior and detect changes like that, so I'm pulled toward testing both.
having an axes_test
function could be good too, to print out detailed information in the case that it's hard to glean from the test failure what went wrong, due to overloading of show
. e.g.
julia> @test 1:3 === OffsetArrays.IdOffsetRange(1:3)
Test Failed at none:1
Expression: 1:3 === OffsetArrays.IdOffsetRange(1:3)
Evaluated: 1:3 === 1:3
ERROR: There was an error during testing
Or perhaps the definition for Base.show(io::IO, r::IdOffsetRange)
should be rewritten, which should be a separate issue.
I think the actual issue is not really that we care that a === OneTo(n)
, but that first(a)
be statically known to be 1 . Is there something like @inferred
but for values?
With #143 is merged, unsure whether this is good to close.
julia> @test OffsetArrays.IdOffsetRange(-2:2) === OffsetArrays.IdOffsetRange(Base.OneTo(5), -3)
Test Failed at REPL[17]:1
Expression: OffsetArrays.IdOffsetRange(-2:2) === OffsetArrays.IdOffsetRange(Base.OneTo(5), -3)
Evaluated: OffsetArrays.IdOffsetRange(-2:2) === OffsetArrays.IdOffsetRange(-2:2)
I guess this is only a developer-sensitive case?
a == IdentityUnitRange(-4:-3), and I don't see the point of doing that instead of a == 3:5
That's because those two shouldn't give the same result, but they do currently thanks to a Julia bug. After all,
julia> a = OffsetArray(-4:-3, -4:-3)
-4:-3 with indices -4:-3
julia> a == -4:-3
false
in just the same way that two dictionaries with equal values but different keys also don't compare as equal.
https://github.com/JuliaLang/julia/pull/30950 was turning into a hydra before I decided that it just needed a reboot, but I never got around to doing that.
This is offtopic, but I'm still not clear about the difference between IdOffsetRange
and IdentityUnitRange
and why we need to introduce it. Would you mind adding a section on this design to the internal docs?
This goes back to Tim's upgrade of the axes. Essentially Base.IdentityUnitRange
is not a lazy wrapper, it only ensures that the axes are the same as the indices it wraps. However IdOffsetRange
is lazy, so it can wrap something like Base.OneTo
and use the fact that arithmetic on Base.OneTo
is faster than that on UnitRange
.
julia> r = Base.IdentityUnitRange(-2:2)
Base.IdentityUnitRange(-2:2)
julia> r2 = OffsetArrays.IdOffsetRange(Base.OneTo(5), -3)
OffsetArrays.IdOffsetRange(-2:2)
julia> @btime $r .+ 1;
3.732 ns (0 allocations: 0 bytes)
julia> @btime $r2 .+ 1;
3.193 ns (0 allocations: 0 bytes)
EDIT: sorry I had not seen jishnub's reply when I wrote this.
julia> pairs(OffsetArrays.IdOffsetRange(Base.OneTo(5), 2))
pairs(::OffsetArrays.IdOffsetRange{Int64,Base.OneTo{Int64}}) with 5 entries:
3 => 3
4 => 4
5 => 5
6 => 6
7 => 7
julia> pairs(Base.IdentityUnitRange(3:7))
pairs(::Base.IdentityUnitRange{UnitRange{Int64}}) with 5 entries:
3 => 3
4 => 4
5 => 5
6 => 6
7 => 7
Here is my guess: IdOffsetRange
and IdentityUnitRange
accomplish the same thing from the perspective of pairs
, etc, but IdOffsetRange
allows you to preserve static information about the underlying array (i.e. in this case, that the first index is 1).
To be fair they are not exactly equivalent if the parent does not start at 1.
julia> pairs(OffsetArrays.IdOffsetRange(5:6, 2))
pairs(::OffsetArrays.IdOffsetRange{Int64,UnitRange{Int64}}) with 2 entries:
3 => 7
4 => 8
julia> pairs(Base.IdentityUnitRange(7:8))
pairs(::Base.IdentityUnitRange{UnitRange{Int64}}) with 2 entries:
7 => 7
8 => 8
The axis of an IdentityUnitRange
is exactly the indices. The axis of an IdOffsetRange
-- as implemented at present -- is the axis of the parent shifted by the offset
.