OffsetArrays.jl icon indicating copy to clipboard operation
OffsetArrays.jl copied to clipboard

Clarify `==`and `===` for `axes` tests

Open goretkin opened this issue 4 years ago • 8 comments

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

goretkin avatar Jun 12 '20 22:06 goretkin

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.

goretkin avatar Jun 12 '20 22:06 goretkin

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?

goretkin avatar Jun 12 '20 23:06 goretkin

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?

johnnychen94 avatar Sep 18 '20 12:09 johnnychen94

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.

timholy avatar Sep 19 '20 10:09 timholy

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?

johnnychen94 avatar Sep 20 '20 09:09 johnnychen94

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)

jishnub avatar Sep 20 '20 10:09 jishnub

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).

goretkin avatar Sep 20 '20 18:09 goretkin

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.

jishnub avatar Sep 21 '20 06:09 jishnub