-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Simplify iteration over ranges in general #27161
Conversation
3739c5e
to
afb66b7
Compare
Probably the specialization for
I think it is safe to assume that we will always land exactly on the boundary of the range then? Will update this PR and simplify the iteration even more :). Edit: done. PR is good to go if the tests are green now. |
e551165
to
b3e48e1
Compare
… largest bits integer type
…; specialization for floats makes no sense, since a StepRange will throw when being constructed with it.
b3e48e1
to
d4b5fdc
Compare
Just to be sure: |
@nanosoldier Need commit bit. |
Hmm... |
@@ -374,7 +374,7 @@ julia> step(range(2.5, stop=10.9, length=85)) | |||
``` | |||
""" | |||
step(r::StepRange) = r.step | |||
step(r::AbstractUnitRange) = 1 | |||
step(r::AbstractUnitRange{T}) where{T} = oneunit(T) |
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 good change, but could we add a ::T
assertion here to ensure that we're in line with the step type we report to OrdinalRange
?
Edit: whoops, sorry, that is oneunit
! It is indeed documented to return T
, which is good enough IMO. I had it backwards from one
.
Ping @ararslan |
There's nothing in the server log indicative of failure so I'm not sure what the problem was, but I restarted the server anyway. @nanosoldier |
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan |
Seems like this is good to go then. |
I'm not too worried about a 20% regression in a deprecated method that's already dog-slow, but I am slightly concerned by the 315% regression in |
I verified locally that its equal (it generates the same > @benchmark perf_findprev(r) setup = (r = rand(Bool, 10000)) #0961ee3599
median time: 55.806 μs (0.00% GC)
> @benchmark perf_findprev(r) setup = (r = rand(Bool, 10000)) #eefa333d01
median time: 55.798 μs (0.00% GC) Also I think |
Great! I confirmed that locally, too. This is good to go. |
This complements #27147 with a bit of refactoring of
StepRange
.AbstractUnitRange
is now identical to the element typeFor. Assume we can land on the boundary of the range exactly, so that we can always iterate over the full range inclusive without overflows.StepRange{<:AbstractFloat}
: assume that we cannot land on the boundary of the range exactlyLinRange
andStepRangeLen
iteration was identical, so they're now combined.Also fixes a bug where one cannot iterate as follows: