Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify ==and === for axes tests #120

Open
goretkin opened this issue Jun 12, 2020 · 8 comments
Open

Clarify ==and === for axes tests #120

goretkin opened this issue Jun 12, 2020 · 8 comments

Comments

@goretkin
Copy link
Contributor

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:

@test axes(v) == (Base.OneTo(1), IdentityUnitRange(-4:-3))

@goretkin
Copy link
Contributor Author

goretkin commented Jun 12, 2020

Making the tests too specific will cause them to fail when they maybe should not on changes like
#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
Copy link
Contributor Author

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?

@johnnychen94
Copy link
Member

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?

@timholy
Copy link
Member

timholy commented Sep 19, 2020

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.

JuliaLang/julia#30950 was turning into a hydra before I decided that it just needed a reboot, but I never got around to doing that.

@johnnychen94
Copy link
Member

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?

@jishnub
Copy link
Member

jishnub commented Sep 20, 2020

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)

@goretkin
Copy link
Contributor Author

goretkin commented Sep 20, 2020

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

@jishnub
Copy link
Member

jishnub commented Sep 21, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants