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

Specialize map(T, ::AbstractRange) for more range types to preserve the result as a range #40746

Closed
wants to merge 3 commits into from

Conversation

jishnub
Copy link
Contributor

@jishnub jishnub commented May 7, 2021

Broadcasting a type over a range currently converts the output to an Array. After this PR, when numerical types are broadcast over ranges of numbers, the output may remain a range

Edit: As broadcasting seemed complicated (the failure is described in #40746 (comment)), this PR only implements map for more range types. Now

julia> map(Int8, Base.OneTo(1))
Base.OneTo(1)

julia> map(Bool, Base.OneTo(1))
true:true

julia> map(Int8, Base.IdentityUnitRange(Base.OneTo(1)))
Base.OneTo(1)

julia> map(Bool, Base.IdentityUnitRange(Base.OneTo(1)))
true:true

This also adds a CartesianIndex constructor that enables

julia> range(CartesianIndex(1, 1), step = CartesianIndex(1, 2), length = 4)
CartesianIndex(1, 1):CartesianIndex(1, 2):CartesianIndex(4, 7)

base/abstractarray.jl Outdated Show resolved Hide resolved
Co-authored-by: Simeon Schaub <[email protected]>
@jishnub
Copy link
Contributor Author

jishnub commented May 8, 2021

Edit: This comment is retained for context, but this PR does not implement broadcasting anymore as this might be breaking as things currently stand.

Using map in broadcasting does not seem to be a great idea, as constructing ranges require more methods to be defined than constructing Vectors do (eg. comparison operators). Test failure happens because:

julia> struct Position <: Integer
           val::Int
       end

julia> Position(x::Position) = x

Previously:

julia> Position.(1:3)
3-element Vector{Position}:
 Position(1)
 Position(2)
 Position(3)

After this PR:

julia> Position.(1:3)
ERROR: <= not defined for Position

In this case perhaps such a change would be too breaking, and this PR might only focus on the additional map methods for ranges?

Incidentally it'd be better for the map to work as well:

julia> map(Position, 1:3)
ERROR: <= not defined for Position

The optimistic approach would be to get the map to work, although I'm not sure if this may be done in general.

@jishnub jishnub changed the title Broadcasting a type over a range may lead to a range as the output Specialize map(T, ::AbstractRange) for more range types to preserve the result as a range May 8, 2021
Comment on lines +1173 to +1178
map(::Type{Int}, r::IdentityUnitRange) = r
# For IdentityUnitRange{<:OneTo}, the map may be evaluated correctly in Base
# We pop the parent range as the result may not always be representable as an IdentityUnitRange
map(::Type{T}, r::IdentityUnitRange{<:OneTo}) where {T<:Real} = map(T, axes1(r))
# this method is for ambiguity resolution
map(::Type{Int}, r::IdentityUnitRange{<:OneTo}) = r
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For r::IdentityUnitRange, given that IdentityUnitRange(r.indices) === r, we could possibly use the deconstruct-and-then-construct alternative to solve ambiguity:

Suggested change
map(::Type{Int}, r::IdentityUnitRange) = r
# For IdentityUnitRange{<:OneTo}, the map may be evaluated correctly in Base
# We pop the parent range as the result may not always be representable as an IdentityUnitRange
map(::Type{T}, r::IdentityUnitRange{<:OneTo}) where {T<:Real} = map(T, axes1(r))
# this method is for ambiguity resolution
map(::Type{Int}, r::IdentityUnitRange{<:OneTo}) = r
Base.map(::Type{T}, r::IdentityUnitRange) where {T<:Real} = identity_or_map(T, axes1(r))
identity_or_map(::Type{Int}, r::AbstractUnitRange) = IdentityUnitRange(r) # the caller `map` thus becomes `identity`
identity_or_map(::Type{T}, r::AbstractUnitRange) = map(T, r)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will stack overflow as axes1(r) === r for Base.IdentityUnitRange if the parent range is not a Base.OneTo

julia> r = Base.IdentityUnitRange(2:3)
Base.IdentityUnitRange(2:3)

julia> map(Int32, r)
ERROR: StackOverflowError:

@dkarrasch dkarrasch added the domain:ranges Everything AbstractRange label May 9, 2021
@jishnub jishnub closed this Mar 20, 2024
@jishnub jishnub deleted the broadcasttyperange branch March 20, 2024 05:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:ranges Everything AbstractRange
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants