-
Notifications
You must be signed in to change notification settings - Fork 152
WIP Return SUnitRange
from indices(::StaticArray)
#150
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
Conversation
Needs testing
Looks sensible so far. |
I somehow broke a macro with this code...
|
whow - so much stack overflow
|
Getting deeper... now it's |
it appears to me that we should make |
The alternative here is to define a new (My eventual "take over the world" goal here is to use a trait to describe if an array is statically sized by asking the question if all it's indices are static, and organize dispatch around this). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯 I think this is a great idea, independent of the question of whether one should support non-1 indexing. (Yes, I want it. For example, in ImageFiltering I'd like to define the Sobel filter, which can be factored along individual dimensions into fixed 3-vectors, as having indices -1:1
.)
(==)(::SUnitRange{1, L}, r::Base.OneTo) where {L} = L == r.stop | ||
(==)(::SUnitRange, ::Base.OneTo) = false | ||
(==)(r::Base.OneTo, ::SUnitRange{1, L}) where {L} = L == r.stop | ||
(==)(::Base.OneTo, ::SUnitRange) = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may also want a variant for generic AbstractUnitRange
.
next(r::SUnitRange, i) = (r[i], i+1) | ||
done(r::SUnitRange{Start, L}, i) where {Start, L} = i > L | ||
|
||
@pure Base.UnitRange{Int}(::SUnitRange{Start, L}) where {Start, L} = Start : (Start + L - 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any particular reason this needs to be @pure
?
done(r::SUnitRange{Start, L}, i) where {Start, L} = i > L | ||
|
||
@pure Base.UnitRange{Int}(::SUnitRange{Start, L}) where {Start, L} = Start : (Start + L - 1) | ||
@pure Base.Slice(::SUnitRange{Start, L}) where {Start, L} = Base.Slice(Start : (Start + L - 1)) # TODO not optimal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not wrong but is perhaps pushing the limits. Base.Slice
is a (deliberately) limited version of IdentityRange, and we only say that two arrays are equal if they have both the same values and the same indices. Since this is a constructor rather than a conversion, you might be able to get away with this, but I'd also add
convert(::Type{Base.Slice}, r::SUnitRange{1,L}) = Base.Slice(r)
convert(::Type{Base.Slice}, r::SUnitRange) = error("$r cannot be converted to Base.Slice, consider `Base.Slice(r)` instead")
(convert
has an implicit promise of preserving at least approximate equality relationships, see the many discussions about whether it's OK to strip physical units e.g., convert(Int, 3meter)
.)
@pure Base.UnitRange{Int}(::SUnitRange{Start, L}) where {Start, L} = Start : (Start + L - 1) | ||
@pure Base.Slice(::SUnitRange{Start, L}) where {Start, L} = Base.Slice(Start : (Start + L - 1)) # TODO not optimal | ||
|
||
function Base.checkindex(::Type{Bool}, ::SUnitRange{Start, L}, r::UnitRange{Int}) where {Start, L} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any particular reason not to define one method for AbstractUnitRange
?
Oh, wow, I see what you mean. Yikes, this is indeed an issue. I think your plan to define a whole new static type that subtypes from |
Cool - thanks Tim for the feedback! You've confirmed everything I thought you would say ;) |
I take predictability as a complement. (In great contrast to my current president...) |
I'm really interested in using Also, is there a reason why |
@c42f, thoughts on whether we are close to ready for doing this?
|
Thanks for your feedback. It sounds that doing things right requires refactoring quite a few things, I'm not sure I have the skills to do this. However a quick way to get what I need would be to start from the Would that make sense? |
Last I looked, I just had the general feeling that these changes are both highly desirable and have the potential to be somewhat breaking. Other than that, I feel like I don't have any special insight on how hard it would be to do this, and I'd only gain some by trying to do the actual work ;-) |
I'll close this PR because:
julia> versioninfo()
Julia Version 0.6.0
Commit 9036443 (2017-06-19 13:05 UTC)
Platform Info:
OS: Linux (x86_64-pc-linux-gnu)
CPU: AMD Ryzen 7 2700X Eight-Core Processor
WORD_SIZE: 64
BLAS: libopenblas (USE64BITINT DYNAMIC_ARCH NO_AFFINITY Barcelona)
LAPACK: libopenblas64_
LIBM: libopenlibm
LLVM: libLLVM-3.9.1 (ORCJIT, generic)
julia> m = @SMatrix rand(3,3)
3×3 StaticArrays.SArray{Tuple{3,3},Float64,2,9}:
0.277928 0.778035 0.645978
0.315307 0.682505 0.176565
0.984043 0.505622 0.843649
julia> indices(m)
(Base.OneTo(3), Base.OneTo(3)) julia> versioninfo()
Julia Version 1.7.2
Commit bf53498635 (2022-02-06 15:21 UTC)
Platform Info:
OS: Linux (x86_64-pc-linux-gnu)
CPU: AMD Ryzen 7 2700X Eight-Core Processor
WORD_SIZE: 64
LIBM: libopenlibm
LLVM: libLLVM-12.0.1 (ORCJIT, znver1)
julia> m = @SMatrix rand(3,3)
3×3 SMatrix{3, 3, Float64, 9} with indices SOneTo(3)×SOneTo(3):
0.994084 0.435498 0.579111
0.120314 0.935061 0.435616
0.0265949 0.0811534 0.772815
julia> axes(m)
(SOneTo(3), SOneTo(3)) |
Needs testing.
Eventually we might be able to move to generic indexing like
OffsetArrays
. But even without that, it's nice to be able to discuss and introspect the indices (at compile time).@timholy are there some caveats to doing this? (I already ran into a few surprising things - perhaps you know of any other gotchas?)