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

OffsetVector(1:10, 4) should be a subtype of AbstractRange #266

Closed
GlenHertz opened this issue Sep 16, 2021 · 11 comments
Closed

OffsetVector(1:10, 4) should be a subtype of AbstractRange #266

GlenHertz opened this issue Sep 16, 2021 · 11 comments

Comments

@GlenHertz
Copy link

Is there a way to apply an offset to a Range such that the type is still a descendent of AbstractRange?

For keeping types precise I'd like the following relationships to be true:

@test 1:10 isa AbstractRange  # passes
@test OffsetVector(1:10, -1) isa AbstractRange #fails

Instead the second line gets converted to an AbstractVector.
This causes problems trying to use OffsetArrays in place of regular Ranges.
Perhaps the constructor should be called OffsetRange if dealing with ranges.

@timholy
Copy link
Member

timholy commented Sep 16, 2021

What you're asking for isn't possible, but there's an internal type,

julia> OffsetArrays.IdOffsetRange(1:10, -1)
OffsetArrays.IdOffsetRange(values=0:9, indices=0:9)

julia> OffsetArrays.IdOffsetRange(values=1:10, indices=-4:5)
OffsetArrays.IdOffsetRange(values=1:10, indices=-4:5)

which is an AbstractRange. We use it for constructing the OffsetArray axes, but you may be successful in using it in applications (let us know how it works).

If this works, we could consider making it public. I'd be opposed to having OffsetArray(args...) do anything other than build a "real" OffsetArray, but we could introduce offsetarray(args...) that would dispatch to IdOffsetRange when appropriate.

@jishnub
Copy link
Member

jishnub commented Sep 16, 2021

@GlenHertz Could you give an example of what problem this causes? Generally I've only seen performance issues if the wrapper doesn't have the same supertype, although certain functions with specialized argument types might fail if they receive an OffsetArray instead of an AbstractRange.

@GlenHertz
Copy link
Author

Thanks for the help! It causes dispatch issues. Briefly, I have this pattern:

function foo(a::AbstractVector, ...)
    # do some stuff to resample uniformly
    b = resample_stuff(a, ...) # b isa AbstractRange
    # call main algorithm    
    foo(b, ...)
end
function foo(a::AbstractRange, ...)
    # main algorithm that works on AbstractRanges
end

# data
s1 = 1:10
# Window of interest:
r = 3:8
# Create a window over r:
w = @view(s1[r])
# Indices should stay as original values:
w2 = OffsetVector(w, firtsindex(r)-1)
foo(w2, ...other_stuff) # w2 should be an AbstractRange and it shouldn't be resampled but just call the main algorithm

I tried using IdOffsetRange but it changes the original values of the range as well as the indicies (which isn't desired):

julia> a = OffsetVector(1:10, -1)
1:10 with indices 0:9

julia> b = OffsetArrays.IdOffsetRange(1:10, -1)
OffsetArrays.IdOffsetRange(values=0:9, indices=0:9)

julia> @test a[2] == b[2]
Test Failed at REPL[55]:1
  Expression: a[2] == b[2]
   Evaluated: 3 == 2
ERROR: There was an error during testing

The values become 0:9 instead of staying 1:10 so it isn't quite doing what I want.

@jishnub
Copy link
Member

jishnub commented Sep 16, 2021

You would want to use it with the values and indices specified, as

julia> OffsetArrays.IdOffsetRange(values=1:10, indices=0:9)
OffsetArrays.IdOffsetRange(values=1:10, indices=0:9)

julia> OffsetArrays.IdOffsetRange(values=1:10, indices=0:9) |> UnitRange
1:10

julia> OffsetArrays.IdOffsetRange(values=1:10, indices=0:9) |> axes
(OffsetArrays.IdOffsetRange(values=0:9, indices=0:9),)

julia> OffsetArrays.IdOffsetRange(values=1:10, indices=0:9) |> axes |> first |> UnitRange
0:9

Using it with arguments instead of keyword arguments is a little different from how an OffsetArray is constructed:

julia> OffsetArrays.IdOffsetRange((1:10) .+ 1, -1) # we need to add the negative of the offset to the values
OffsetArrays.IdOffsetRange(values=1:10, indices=0:9)

julia> OffsetArrays.IdOffsetRange((1:10) .+ 1, -1) |> UnitRange
1:10

julia> OffsetArrays.IdOffsetRange((1:10) .+ 1, -1) |> axes |> first |> UnitRange
0:9

The keyword-argument version of the constructor automatically does this for you, and is more readable.

@GlenHertz
Copy link
Author

Yes that will work. Thanks for the tips!

@timholy
Copy link
Member

timholy commented Sep 17, 2021

I'll close this, and open an issue about whether we should export IdOffsetRange.

@GlenHertz
Copy link
Author

After using this for a bit the one issue I have with IdOffsetRange is it only supports AbstractUnitRange where I'd like to be able to use it also for AbstractRange.

@jishnub
Copy link
Member

jishnub commented Sep 20, 2021

We currently don't have an offset type that is a subtype of AbstractRange. Would it be possible to rewrite your methods to not be too specific in the arguments that they accept? Eg. something like:

function foo(a::AbstractVector, ...)
    # do some stuff to resample uniformly
    b = resample_stuff(a, ...) # b isa AbstractRange
    # call main algorithm    
    foo_main(b, ...)
end

function foo_main(a::AbstractVector, ...)
    # main algorithm that works on AbstractRanges or wrapper types
end

@timholy
Copy link
Member

timholy commented Sep 20, 2021

I'd accept a pull request to add more AbstractRange types. You could model it on IdOffsetRange if you wanted.

@GlenHertz
Copy link
Author

For this to work well I believe enhancements to base Julia would also need to be made:

julia> searchsorted((OffsetArrays.IdOffsetRange(1:10, -2)), 3.3)
ERROR: ArgumentError: offset arrays are not supported but got an array with index other than 1
Stacktrace:
 [1] require_one_based_indexing
   @ ./abstractarray.jl:103 [inlined]

So I'm not sure how viable this would be as quite a few places require 1-based indexing.

@timholy
Copy link
Member

timholy commented Sep 22, 2021

All that is potentially fixable, but of course more work. Ranges really have 1-based indexing baked in more than most places.

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

3 participants