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

::OrdinalRange .* ::Integer returns a StepRangeLen (which is no longer OrdinalRange) #57196

Open
arnaud-ma opened this issue Jan 29, 2025 · 3 comments
Labels
ranges Everything AbstractRange

Comments

@arnaud-ma
Copy link

arnaud-ma commented Jan 29, 2025

For example

julia> typeof((1:2:10) .+ 4) # works fine
StepRange{Int64, Int64}

julia> typeof((1:2:10) .* 4)
StepRangeLen{Int64, Int64, Int64, Int64}

It can be an issue for functions that only requires OrdinalRange. For example

julia> CartesianIndices((1:10) .* 4, 1:10)
ERROR: MethodError: no method matching CartesianIndices(::StepRangeLen{Int64, Int64, Int64, Int64}, ::UnitRange{Int64})

I think the easy fix would be to write this:

julia/base/broadcast.jl

Lines 1139 to 1140 in e65af91

broadcasted(::DefaultArrayStyle{1}, ::typeof(+), r::OrdinalRange, x::Integer) = range(first(r) + x, last(r) + x, step=step(r))
broadcasted(::DefaultArrayStyle{1}, ::typeof(+), x::Integer, r::OrdinalRange) = range(x + first(r), x + last(r), step=step(r))

but for typeof(*), and by also multiplying the step to have the same behavior.

@martinholters
Copy link
Member

I think the problem is that (1:2:10) .* 0 cannot be represented as a StepRange, so for type stability, we always return a StepRangeLen, even if the result could be represented as a StepRange.

@mikmoore
Copy link
Contributor

mikmoore commented Jan 30, 2025

That it should return a StepRangeLen for type stability seems unavoidable. But I don't think that's necessarily the end of the story as far as CartesianIndices is concerned.

Once again we encounter the pitfalls of types-as-traits. CartesianIndices is too strict here. It could at least attempt to convert a StepRangeLen{<:Integer} (which is an OrdinalRange in every criterion except <:) to a StepRange. One could go so far as to suggest it should support AbstractRange{<:Integer} generally -- I don't see profound harm in a CartesianIndices with a step of 0 in one or more axes.

@nsajko nsajko added the ranges Everything AbstractRange label Jan 30, 2025
@martinholters
Copy link
Member

Maybe convert(::Type{StepRange{...}}, ::StepRangeLen) could be defined, which would throw an InexactError when appropriate?

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

No branches or pull requests

4 participants