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

Error when specifying the range of an index with a UnitRange #175

Open
jgidi opened this issue May 15, 2023 · 4 comments
Open

Error when specifying the range of an index with a UnitRange #175

jgidi opened this issue May 15, 2023 · 4 comments
Labels

Comments

@jgidi
Copy link

jgidi commented May 15, 2023

Hi!

If we perform an operation with @tullio that needs to specify the range of an index manually, as follows:

using Tullio

N = 10
A = rand(5)

@tullio B[i, j] := A[i] (j in 1:N)

It errors with a message:
ERROR: MethodError: no method matching similar(::Vector{Float64}, ::Type{Float64}, ::Tuple{Base.OneTo{Int64}, UnitRange{Int64}})

I have found two workarounds so far.

  1. Write the value of N explicitly (which is very limiting):
@tullio B[i, j] := A[i] (j in 1:10)
  1. Use Base.OneTo instead of a UnitRange:
@tullio B[i, j] := A[i] (j in Base.OneTo(N))

This seems to be because similar does not accept UnitRanges as axes, and probably Tullio made some conversion in the first case when N was provided explicitly, but I don't know why the same conversion is not performed when the range is given as 1:N. Also, I can confirm this happens at least for the julia versions 1.6.7 and 1.9.0.

While the second workaround circumvents the problem completely, I think this behavior is odd and worth mentioning.

Thanks in advance!

@mcabbott
Copy link
Owner

mcabbott commented May 15, 2023

Yes this really ought to work. It can see the 1 so it should be smart enough to make a OneTo for you. (In fact I thought there was already code for this, but must be wrong.)

Note that if you write 2:N it has to let this though, and make an OffsetArray:

julia> using OffsetArrays

julia> @tullio B[i, j] := A[i] (j in 2:N)
5×9 OffsetArray(::Matrix{Float64}, 1:5, 2:10) with eltype Float64 with indices 1:5×2:10:
 0.580193  0.580193  0.580193  0.580193  0.580193  0.580193  0.580193  0.580193  0.580193
 ...

@jgidi
Copy link
Author

jgidi commented May 15, 2023

I can effectively reproduce your example. It seems that the code for that functionality only gets available after loading OffsetArrays, even for a UnitRange starting at 1.

After using OffsetArrays my first example works, but an OffsetArray is returned, which shouldn't be the case:

julia> using Tullio                                                    
                                                                       
julia> N = 10;                                                         
                                                                       
julia> A = rand(5);

julia> @tullio B[i, j] := A[i] (j in 1:N)
ERROR: MethodError: no method matching similar(::Vector{Float64}, ::Type{Float64}, ::Tuple{Base.OneTo{Int64}, UnitRange{Int64}})
...

julia> using OffsetArrays

julia> @tullio B[i, j] := A[i] (j in 1:N)
5×10 OffsetArray(::Matrix{Float64}, 1:5, 1:10) with eltype Float64 with indices 1:5×1:10:
 0.897518   0.897518   0.897518     0.897518   0.897518   0.897518
 ...

@mcabbott
Copy link
Owner

OK there is code to make OneTo, but it's too restrictive:

Tullio.jl/src/macro.jl

Lines 633 to 637 in 5d31055

if isexpr(r, :call) && r.args[1] == :(:) && length(r.args) == 3
# for a literal range, write OneTo(10) or 0:9 directly into constraints
if r.args[2] == 1 && r.args[3] isa Integer
push!(v, :(Base.OneTo($(r.args[3]))))
continue

julia> r = :(1:N)
:(1:N)

julia> r.args[1] == :(:) && length(r.args) == 3
true

julia> r.args[2] == 1
true

julia> r.args[3] isa Integer
false

julia> @tullio B[i, j] := A[i] (j in 1:10)  # with literal 10, this path makes OneTo(10):
5×10 Matrix{Float64}:
 0.573777  0.573777  0.573777 ...

@jgidi
Copy link
Author

jgidi commented May 15, 2023

This may seem like an odd question, but do we really need to check that r.args[3] isa Integer? I just tested removing that check on L635 and everything seems to work great.
Obviously, if we pass a non-integer N it will fail,

julia> N = 5.5;

julia> @tullio B[i, j] := A[i] (j in 1:N)
ERROR: MethodError: no method matching Base.OneTo(::Float64)
...

but I think that error is pretty obvious to the user (at least more than it is now) and I don't think there would be side effects besides yielding the same error if a literal Float is passed as 1:5.5.

If you like the idea I can propose this (very small) PR.

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

No branches or pull requests

2 participants