Make `StepRangeLen` in operations which may produce zero step #40320

mcabbott · 2021-04-02T17:42:52Z

This allows calculations with integer ranges leading to zero step to work:

julia> 0 * (1:3)
StepRangeLen(0, 0, 3)

julia> (1:3) - (1:3)
StepRangeLen(0, 0, 3)

julia> exp.(ans)
3-element Vector{Float64}:
 1.0
 1.0
 1.0

Will this have awful side effects elsewhere?

Note that it does not change how range behaves, nor allow you to enter 1:0:1. Many floating point ranges already use StepRangeLen and hence already allow zero step, it does change their printing to something which parses:

julia> range(1, 5, step=2) |> typeof
StepRange{Int64, Int64}

 julia> 0 * (1:2.0:10)
StepRangeLen(0.0, 0.0, 5)

Closes #40317. Closes #10391.

base/range.jl

simeonschaub · 2021-04-03T12:24:42Z

Would it be good to run PkgEval here? I could imagine that some people are implicitly relying on these returning a StepRange.

simeonschaub · 2021-04-03T17:14:26Z

@nanosoldier runtests(ALL, vs = ":master")

nanosoldier · 2021-04-03T20:23:26Z

Something went wrong when running your job:

NanosoldierError: failed to run tests: TaskFailedException:
failed process: Process(`docker stop ExportAll-WED15I1v`, ProcessExited(1)) [1]

pipeline_error at ./process.jl:525 [inlined]
#run#596 at ./process.jl:440
run at ./process.jl:438 [inlined]
kill_container at /storage/pkgeval/dev/PkgEval/src/run.jl:364 [inlined]
stop at /storage/pkgeval/dev/PkgEval/src/run.jl:167 [inlined]
#36 at /storage/pkgeval/dev/PkgEval/src/run.jl:234 [inlined]
lock at ./lock.jl:161
macro expansion at /storage/pkgeval/dev/PkgEval/src/run.jl:231 [inlined]
#35 at ./task.jl:356
wait at ./task.jl:267 [inlined]
fetch at ./task.jl:282 [inlined]
#run_sandboxed_test#30 at /storage/pkgeval/dev/PkgEval/src/run.jl:258
macro expansion at /storage/pkgeval/dev/PkgEval/src/run.jl:584 [inlined]
#50 at ./task.jl:356

Logs and partial data can be found here
cc @maleadt

maleadt · 2021-04-04T09:35:34Z

Docker hiccup. Let's try again:

@nanosoldier runtests(ALL, vs = ":master")

nanosoldier · 2021-04-04T15:28:43Z

Your package evaluation job has completed - possible new issues were detected. A full report can be found here. cc @maleadt

simeonschaub · 2021-04-04T16:06:58Z

The failures look mostly real.

The AbstractFFT failures are interesting, since it looks like this change introduced some numerical instabilities with operations involving floating points with integer ranges. Could we perhaps promote to TwicePrecision if a StepRange is multiplied by a float?

InfiniteArrays would be good to look into as well, but I am guessing it wouldn't be too hard to fix the breakage on their side.

The others mostly look like their tests are just a little to strict and explicitly test the type of a range, so I don't think we need to worry too much about those.

mcabbott · 2021-04-12T00:17:48Z

I think I fixed the AbstractFFTs test, which was producing this:

julia> using AbstractFFTs

julia> x = fftfreq(5);

julia> fftshift(fftfreq(5)) |> collect
5-element Vector{Float64}:
 -0.4
 -0.2
  0.0
  0.20000000000000007
  0.4

julia> (x.n_nonnegative-x.n:x.n_nonnegative-1)
-2:2

julia> (x.n_nonnegative-x.n:x.n_nonnegative-1)*x.multiplier
-0.4:0.2:0.4

julia> typeof(ans)
StepRangeLen{Float64, Float64, Float64}

# My fix
julia> @eval Base.Broadcast broadcasted(::DefaultArrayStyle{1}, ::typeof(*), r::AbstractRange, x::AbstractFloat) = Base.range_start_step_length(first(r)*x, step(r)*x, length(r))

julia> (x.n_nonnegative-x.n:x.n_nonnegative-1)*x.multiplier |> typeof
StepRangeLen{Float64, Base.TwicePrecision{Float64}, Base.TwicePrecision{Float64}}

But now there are other problems... hope nobody is in a rush.

simeonschaub · 2021-04-29T21:52:14Z

Any news on this? Not in a rush, but it would be nice to get this into 1.7, since it could be argued that this is really a bugfix. Let me know if there's anything I can help with.

mcabbott · 2021-04-30T04:47:57Z

base/broadcast.jl

+broadcasted(::DefaultArrayStyle{1}, ::typeof(*), r::AbstractRange, x::Number) = StepRangeLen(first(r)*x, step(r)*x, length(r))
+broadcasted(::DefaultArrayStyle{1}, ::typeof(*), r::StepRangeLen{T}, x::Number) where {T} = StepRangeLen{typeof(T(r.ref)*x)}(r.ref*x, r.step*x, length(r), r.offset)
 broadcasted(::DefaultArrayStyle{1}, ::typeof(*), r::LinRange, x::Number) = LinRange(r.start * x, r.stop * x, r.len)
+broadcasted(::DefaultArrayStyle{1}, ::typeof(*), r::OrdinalRange{<:Integer}, x::AbstractFloat) = Base.range_start_step_length(first(r)*x, x, length(r))


The last method here handles the case needed for AbstractFFTs tests, above, of float * UnitRange -> range with twiceprecision.

And (-2:2) * 0.0 works because for floats range_start_step_length does sometimes allow zero step:

julia> Base.range_start_step_length(1.0, 0, 5) StepRangeLen(1.0, 0.0, 5) julia> Base.range_start_step_length(1, 0, 5) # goes to 1st method not 4th ERROR: ArgumentError: step cannot be zero

But probably it could just be OrdinalRange. Are there other weird cases we should think about?

I wonder whether Base.range_start_step_length(1, 0, 5) should just return a StepRangeLen. Do you expect that to be too breaking?

I haven't thought about it. Was hoping to change as little as possible, focused on calculations that may produce zero step, rather than constructors.

This one is called by range(1, step=0, length=10) which would otherwise make an integer StepRange. Whereas range(1, 1, length=10) works, and makes a floating point range. It is a bit weird that some constructors work and some don't, but this isn't changed here.

Agree, it's probably better not to change this behavior in this PR. Could you just add a short comment here explaining why we can't use range_start_step_length?

Sure, see what you think of 56d9c27

Looks good! Let's run PkgEval one more time once tests pass, but otherwise this should be good to go.

base/range.jl

simeonschaub · 2021-04-30T20:30:52Z

@nanosoldier runtests(["AbstractFFTs", "ArrayLayouts", "BlockArrays", "CUTEst", "Cambrian", "CustomUnitRanges", "DimensionalData", "Hecke", "IdentityRanges", "ImageDistances", "InfiniteArrays", "InverseLaplace", "LoopThrottle", "ProximalAlgorithms"], vs = ":master")

nanosoldier · 2021-04-30T21:13:46Z

Your package evaluation job has completed - possible new issues were detected. A full report can be found here. cc @maleadt

mcabbott · 2021-04-30T21:24:02Z

Besides === tests:

IdentityRanges:

Test Failed at /home/pkgeval/.julia/packages/IdentityRanges/liKDH/test/runtests.jl:3
  Expression: isempty(detect_ambiguities(IdentityRanges, Base, Core))
   Evaluated: isempty(Tuple{Method, Method}[(broadcasted(::Base.Broadcast.DefaultArrayStyle{1}, ::typeof(*), x::Number, r::IdentityRange) in IdentityRanges at /home/pkgeval/.julia/packages/IdentityRanges/liKDH/src/IdentityRanges.jl:139, broadcasted(::Base.Broadcast.DefaultArrayStyle{1}, ::typeof(*), x::AbstractFloat, r::OrdinalRange) in Base.Broadcast at broadcast.jl:1161), (broadcasted(::Base.Broadcast.DefaultArrayStyle{1}, ::typeof(*), r::IdentityRange, x::Number) in IdentityRanges at /home/pkgeval/.julia/packages/IdentityRanges/liKDH/src/IdentityRanges.jl:138, broadcasted(::Base.Broadcast.DefaultArrayStyle{1}, ::typeof(*), r::OrdinalRange, x::AbstractFloat) in Base.Broadcast at broadcast.jl:1168)])

That ambiguity might be avoided if you modified range_start_step_length instead.

BlockArrays:

Special broadcast: Test Failed at /home/pkgeval/.julia/packages/BlockArrays/aLe8n/test/test_blockbroadcast.jl:114
  Expression: broadcast(*, 2, v) isa BlockVector{Int, Vector{StepRange{Int, Int}}}
   Evaluated: [2, 4, 6, 8, 10, 12, 14] isa BlockVector{Int64, Vector{StepRange{Int64, Int64}}}

Maybe that's roughly like ===, something isn't giving a StepRange anymore.

simeonschaub · 2021-04-30T21:44:09Z

IdentityRanges:

Test Failed at /home/pkgeval/.julia/packages/IdentityRanges/liKDH/test/runtests.jl:3
  Expression: isempty(detect_ambiguities(IdentityRanges, Base, Core))
   Evaluated: isempty(Tuple{Method, Method}[(broadcasted(::Base.Broadcast.DefaultArrayStyle{1}, ::typeof(*), x::Number, r::IdentityRange) in IdentityRanges at /home/pkgeval/.julia/packages/IdentityRanges/liKDH/src/IdentityRanges.jl:139, broadcasted(::Base.Broadcast.DefaultArrayStyle{1}, ::typeof(*), x::AbstractFloat, r::OrdinalRange) in Base.Broadcast at broadcast.jl:1161), (broadcasted(::Base.Broadcast.DefaultArrayStyle{1}, ::typeof(*), r::IdentityRange, x::Number) in IdentityRanges at /home/pkgeval/.julia/packages/IdentityRanges/liKDH/src/IdentityRanges.jl:138, broadcasted(::Base.Broadcast.DefaultArrayStyle{1}, ::typeof(*), r::OrdinalRange, x::AbstractFloat) in Base.Broadcast at broadcast.jl:1168)])

That ambiguity might be avoided if you modified range_start_step_length instead.

That should be easily fixable on IdentityRanges' side, so I am not too worried about that.

BlockArrays:

Special broadcast: Test Failed at /home/pkgeval/.julia/packages/BlockArrays/aLe8n/test/test_blockbroadcast.jl:114
  Expression: broadcast(*, 2, v) isa BlockVector{Int, Vector{StepRange{Int, Int}}}
   Evaluated: [2, 4, 6, 8, 10, 12, 14] isa BlockVector{Int64, Vector{StepRange{Int64, Int64}}}

Maybe that's roughly like ===, something isn't giving a StepRange anymore.

Yeah, it's just explicitly testing the type, so that should be fine as well.

mcabbott · 2021-05-13T23:17:22Z

Pre-1.7 bump?

simeonschaub · 2021-05-14T04:46:46Z

Just realized that it might also be good to have a news entry for this, would you mind adding one? Otherwise will merge later if there are no objections.

mcabbott · 2021-05-14T14:47:10Z

Sure. I added something, perhaps too verbose?

simeonschaub · 2021-05-14T14:47:59Z

No, looks good.

…ang#40320)

vtjnash reviewed Apr 3, 2021

View reviewed changes

base/range.jl Outdated Show resolved Hide resolved

vtjnash approved these changes Apr 3, 2021

View reviewed changes

vtjnash added the status:merge me PR is reviewed. Merge when all tests are passing label Apr 3, 2021

simeonschaub removed the status:merge me PR is reviewed. Merge when all tests are passing label Apr 4, 2021

mcabbott added 4 commits April 11, 2021 09:53

make StepRangeLen more often, to allow zero step

86a4403

remove some === tests

11a9957

tidy up

0eb2160

new AbstractFloat methods

33d6e30

mcabbott force-pushed the zerostep branch from 529beb2 to 33d6e30 Compare April 11, 2021 18:31

rm two methods

709031e

mcabbott added 4 commits April 29, 2021 21:07

solve an ambiguity

4028cfc

maybe this will fix it

2bfe5ec

tests from JuliaLang#40309

1019b3a

one more...

efd9ba1

mcabbott force-pushed the zerostep branch from 38c5643 to efd9ba1 Compare April 30, 2021 02:50

mcabbott commented Apr 30, 2021

View reviewed changes

mcabbott added 2 commits April 30, 2021 11:18

tidy

c54dc5f

notes + line wrap

611cd63

mcabbott force-pushed the zerostep branch from 56d9c27 to 611cd63 Compare April 30, 2021 16:17

simeonschaub added the needs pkgeval Tests for all registered packages should be run with this change label Apr 30, 2021

simeonschaub reviewed Apr 30, 2021

View reviewed changes

base/range.jl Outdated Show resolved Hide resolved

restrict printing to StepRangeLen

15d226b

simeonschaub approved these changes Apr 30, 2021

View reviewed changes

simeonschaub mentioned this pull request Apr 30, 2021

should range(1, step=2, length=3)be a StepRangeLen? #40672

Open

simeonschaub added domain:ranges Everything AbstractRange and removed needs pkgeval Tests for all registered packages should be run with this change labels Apr 30, 2021

add news item

93004fa

simeonschaub added the status:merge me PR is reviewed. Merge when all tests are passing label May 14, 2021

Merge remote-tracking branch 'origin/master' into zerostep

f10b89d

simeonschaub merged commit 5aca7a3 into JuliaLang:master May 15, 2021

mcabbott deleted the zerostep branch May 15, 2021 16:49

This was referenced May 17, 2021

Error with Float64 * StepRange{Int,Int} with negative step #40846

Closed

Fix #40846, wrong results from Float64 .* StepRange #40847

Merged

simeonschaub removed the status:merge me PR is reviewed. Merge when all tests are passing label May 21, 2021

shirodkara pushed a commit to shirodkara/julia that referenced this pull request Jun 9, 2021

Make StepRangeLen in operations which may produce zero step (JuliaL…

803f185

…ang#40320)

johanmon pushed a commit to johanmon/julia that referenced this pull request Jul 5, 2021

Make StepRangeLen in operations which may produce zero step (JuliaL…

dbdf40b

…ang#40320)

KristofferC mentioned this pull request Jul 5, 2021

fix tests for 1.7 for change when StepRangeLen is produced JuliaArrays/CustomUnitRanges.jl#21

Merged

dlfivefifty mentioned this pull request Jul 8, 2021

Julia v1.7: Multiplication broken with BigInt ranges #41517

Closed

jameskermode mentioned this pull request Apr 7, 2022

Update type of CubicBSpline for StepRangeLen introduced in Julia 1.7 JuliaMolSim/JuLIP.jl#153

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `StepRangeLen` in operations which may produce zero step #40320

Make `StepRangeLen` in operations which may produce zero step #40320

mcabbott commented Apr 2, 2021

simeonschaub commented Apr 3, 2021

simeonschaub commented Apr 3, 2021

nanosoldier commented Apr 3, 2021

maleadt commented Apr 4, 2021

nanosoldier commented Apr 4, 2021

simeonschaub commented Apr 4, 2021

mcabbott commented Apr 12, 2021

simeonschaub commented Apr 29, 2021

mcabbott Apr 30, 2021

simeonschaub Apr 30, 2021

mcabbott Apr 30, 2021

simeonschaub Apr 30, 2021

mcabbott Apr 30, 2021

simeonschaub Apr 30, 2021

simeonschaub commented Apr 30, 2021

nanosoldier commented Apr 30, 2021

mcabbott commented Apr 30, 2021

simeonschaub commented Apr 30, 2021

mcabbott commented May 13, 2021

simeonschaub commented May 14, 2021

mcabbott commented May 14, 2021

simeonschaub commented May 14, 2021

Make StepRangeLen in operations which may produce zero step #40320

Make StepRangeLen in operations which may produce zero step #40320

Conversation

mcabbott commented Apr 2, 2021

simeonschaub commented Apr 3, 2021

simeonschaub commented Apr 3, 2021

nanosoldier commented Apr 3, 2021

maleadt commented Apr 4, 2021

nanosoldier commented Apr 4, 2021

simeonschaub commented Apr 4, 2021

mcabbott commented Apr 12, 2021

simeonschaub commented Apr 29, 2021

mcabbott Apr 30, 2021

Choose a reason for hiding this comment

simeonschaub Apr 30, 2021

Choose a reason for hiding this comment

mcabbott Apr 30, 2021

Choose a reason for hiding this comment

simeonschaub Apr 30, 2021

Choose a reason for hiding this comment

mcabbott Apr 30, 2021

Choose a reason for hiding this comment

simeonschaub Apr 30, 2021

Choose a reason for hiding this comment

simeonschaub commented Apr 30, 2021

nanosoldier commented Apr 30, 2021

mcabbott commented Apr 30, 2021

simeonschaub commented Apr 30, 2021

mcabbott commented May 13, 2021

simeonschaub commented May 14, 2021

mcabbott commented May 14, 2021

simeonschaub commented May 14, 2021

Make `StepRangeLen` in operations which may produce zero step #40320

Make `StepRangeLen` in operations which may produce zero step #40320