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

allow `range(start, stop; ...)` #28708

Merged
merged 1 commit into from Oct 20, 2018

Conversation

9 participants
@JeffBezanson
Copy link
Member

commented Aug 16, 2018

This was discussed in #25896, and can be added now that deprecations are removed. This probably could have been in 1.0, and so might be ok to put in a point release. Thoughts? Otherwise it can go in 1.1.

@tknopp

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2018

Yes please add that and slip it into 1.0.1. The stop thing really was a little odd when I ported Winston to 1.0.

@fredrikekre

This comment has been minimized.

Copy link
Member

commented Aug 16, 2018

Should be v1.1.

@fredrikekre fredrikekre added this to the 1.1 milestone Aug 16, 2018

@StefanKarpinski

This comment has been minimized.

Copy link
Member

commented Aug 17, 2018

Let's not commit a flagrant violation of SemVer in literally our first post-1.0 release. @JeffBezanson, this is why I suggested that a fairly quick 1.1 on the order of 1-2 months might be in order because there are a number of little features like this that were unblocked by the removal of deprecations and which were not implemented yet in 1.0 on account of the accelerated time frame of the release.

@@ -76,6 +76,34 @@ julia> range(1, step=5, stop=100)
range(start; length::Union{Integer,Nothing}=nothing, stop=nothing, step=nothing) =

This comment has been minimized.

Copy link
@mbauman

mbauman Aug 30, 2018

Member

Maybe remove the docstring from this method? Or simplify it significantly? The two are essentially identical and will just end up looking repetitive in the docs.

@jlperla

This comment has been minimized.

Copy link
Contributor

commented Aug 31, 2018

Is this PR (or an equivalent interface) almost certainly going into the v1.1 release? To the extent that it can be discussed wither Compat.jl might support it?

@stillyslalom

This comment has been minimized.

Copy link
Contributor

commented Sep 1, 2018

Any chance of including range(start, stop) = range(start, stop, length=50) to mimic the default-length behavior of Matlab and Numpy? I'm happy to put together the PR and docs if the core devs are open to it. It's a convenience function, sure, but it isn't going to hurt anyone.

@jlperla

This comment has been minimized.

Copy link
Contributor

commented Sep 2, 2018

Any chance of including range(start, stop) = range(start, stop, length=50) to mimic the default-length behavior of Matlab and Numpy?

That one I never liked in matlab, personally. I think the worry with the new range is that my guess on the default behavior (if any) is that range(start, stop) = start:stop which gives an appropriately generic step size of 1... But since start:stop is already so clean, I am not sure defaults in range make sense

@StefanKarpinski

This comment has been minimized.

Copy link
Member

commented Sep 3, 2018

Why in the world is the default length 50?! Why not 100? That seems like a better and less surprising default. Defaulting to 50 is just something someone liked once upon a time that has now become enshrined for no reason whatsoever in various systems that copied each other. Let’s break the cycle of an arbitrary default value that never made any sense in the first place.

@jlperla

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2018

@StefanKarpinski is this new interface sufficiently baked into Julia plans that we can try to make a case that it can go into Compat.jl?

This was a strategy brought up by @simonbyrne (though I want to be clear that he didn't endorse it necessarily).

The idea is that if the new range interface is finalized, that we would add it to Compat.jl so that people appalled by the current range interface can opt-in with the Compat.jl interface, at which point it would eventually just smoothly transition to v1.1

And with no semver violations, or conflicting homegrown type piracy between code bases to get the new interface.

@martinholters

This comment has been minimized.

Copy link
Member

commented Sep 3, 2018

Once it's in Base, I see no reason it couldn't be added to Compat.

@stillyslalom

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2018

I don't particularly care about the exact default length, but it's not completely arbitrary: 50 points are enough to capture the behavior of most smooth functions for plotting purposes, and "Break the tyranny of the sensible default value!" isn't an actual argument against default values. But this is a distracting bikeshed, and the PR's already an improvement over the current syntax, so I'll shut up.

@StefanKarpinski

This comment has been minimized.

Copy link
Member

commented Sep 4, 2018

Pre-Compating this seems reasonable. There’s no real debate about what we’re going to do here.

@jlperla

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2018

@StefanKarpinski Does this need to be merged before we can add it to Compat.jl (cc: @Nosferican ) If so, any movement towards merging is appreciated.

@AzamatB

This comment has been minimized.

Copy link
Contributor

commented Sep 9, 2018

Why not 100? That seems like a better and less surprising default

@StefanKarpinski, well, this leads to the proposal: let's make 100 as default. One possible justification for this choice is that the currently range(start, stop) defaults to start:stop, which means that it will never be used, since start:stop is way cleaner syntax, so it will always be given preference to.
We already are defaulting to step=1 with a:b constructs, do we need yet another interface that replicates this behavior? Or maybe we can make more orthogonal design choice and come up with more useful default for range(start, stop)?

Besides defaulting to step=1 is as random as choosing length 100 or 50. But contrary to step=1, defaulting to length=100 has actually a chance of being useful (e.g. when making a quick plot of some smooth function, where you don't care about spacing so long as it captures the smoothness)

TL;DR: choosing the least useful option as default one does not seem reasonable to me.

@JeffBezanson

This comment has been minimized.

Copy link
Member Author

commented Sep 10, 2018

I don't think a default step of 1 is as random as a default length of 100 or 50. Consecutive integers aren't exactly some crazy thing we just made up. I guess it's slightly arbitrary when the arguments are not integer-valued though.

@AzamatB

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2018

@JeffBezanson Yes, it can be quite difficult to recognize when seeing in someone's code something like range(1, 2.9) that it actually just means 1.0:1.0:2.0. And, in fact, one could question the validity of rounding down the stop. Might as well round it up so that range(1, 2.9) will yield 1.0:1.0:3.0 or round it to the nearest integer. This seems to me another arbitrary choice, which can be avoided altogether if length=100 is used.

I guess my point is that the current default has also arbitrary choices baked into it that make it confusing when reading someone's code. Furthermore, it is already covered by more slicker a:b syntax, which brings it's utility down to 0. By contrast, length=100 is maybe only ε more arbitrary, but it actually can be useful.

@jlperla

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2018

Is a default for step or length necessary at all? I am not sure I ever see range(a, b) being useful in generic code, for example, where a:b wouldn't be clearer?

@JeffBezanson

This comment has been minimized.

Copy link
Member Author

commented Sep 10, 2018

A default step of 1 clearly makes sense for 1:n, and we just take it from there. However I could see the case for allowing this only for integers (or maaybe integer-valued floats as well).

We could also require that a length or step keyword argument be passed for range(start, stop).

@StefanKarpinski

This comment has been minimized.

Copy link
Member

commented Sep 10, 2018

The most conservative choice is to require one of them to be passed. That seems fine for now.

@martinholters

This comment has been minimized.

Copy link
Member

commented Oct 4, 2018

I'd still prefer to require either length or step to be given, as @StefanKarpinski proposed. (At least for now; it would leave the option of providing a default in the future.) But if we do want to support range(start, stop), I definitely prefer the current step=1 default over an arbitrary default for length.

@martinholters

This comment has been minimized.

Copy link
Member

commented Oct 4, 2018

AV error looks unrelated but interesting.

@martinholters

This comment has been minimized.

Copy link
Member

commented Oct 5, 2018

Ah, I wasn't even aware we supported range(1, stop=100) (with a default step of 1). Seems slightly inconsistent not to support range(1, 100) then. Deprecating the former at this point is a no-go. I think my preferred route forward would be to not support range(1, 100) and schedule range(1, stop=100) for future deprecation. (I cannot imagine it to be in widespread use, I'd always prefer to write 1:100, but I might be wrong there.)

@JeffBezanson JeffBezanson force-pushed the jb/2argrange branch 2 times, most recently from 540f547 to da3f77d Oct 18, 2018

@JeffBezanson

This comment has been minimized.

Copy link
Member Author

commented Oct 18, 2018

Comments addressed.

@martinholters
Copy link
Member

left a comment

The docstring gives the default step=1 which is not entirely correct. I'm not sure it's worth documenting the different behavior of the one-arg and two-arg versions. Maybe we restrict the documentation to the behavior of the two-arg version (either step or length has to be specified) and only silently keep supporting the old one-arg-with-neither-given?

@@ -71,11 +73,28 @@ julia> range(1, step=5, length=100)
julia> range(1, step=5, stop=100)
1:5:96
julia> range(1, 100)
1:100

This comment has been minimized.

Copy link
@martinholters

martinholters Oct 18, 2018

Member

This doctest fails with the latest changes.

This comment has been minimized.

Copy link
@JeffBezanson

JeffBezanson Oct 18, 2018

Author Member

Oops.

@JeffBezanson JeffBezanson force-pushed the jb/2argrange branch from da3f77d to a1700c0 Oct 19, 2018

@JeffBezanson

This comment has been minimized.

Copy link
Member Author

commented Oct 19, 2018

Aha: the reason the default step of 1 is useful is that it allows returning a UnitRange.

@JeffBezanson JeffBezanson force-pushed the jb/2argrange branch from a1700c0 to 9704320 Oct 19, 2018

@martinholters

This comment has been minimized.

Copy link
Member

commented Oct 19, 2018

Ah, yes. Although that means the default is not really 1:

julia> range(1, stop=5)
1:5

julia> range(1, stop=5, step=1)
1:1:5

I'd expect giving the default explicitly not making any difference at all.

Irrespective of the step=1 default business—should we schedule the stop-as-keyword-arg version for future deprecation?

@JeffBezanson

This comment has been minimized.

Copy link
Member Author

commented Oct 19, 2018

I guess we could change the documentation to say the default is a UnitRange when no step is specified. But, 1:5 == 1:1:5 is true so this is getting rather pedantic.

I don't think range(a, b) and range(a, stop=b) are harmful and I don't see the need to deprecate anything here. In the current state of the PR range(a, b) requires an additional keyword argument, so we have the property that every call to range must have at least one keyword argument, which guarantees that every call site is pretty clear in its meaning.

@JeffBezanson JeffBezanson merged commit fecfbf9 into master Oct 20, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
julia freebsd ci Build done
Details

@JeffBezanson JeffBezanson deleted the jb/2argrange branch Oct 20, 2018

fredrikekre added a commit that referenced this pull request Dec 5, 2018

Addition of NEWS and compat admonitions for important
changes between Julia 1.0 and 1.1, including:

- Custom .css-style for compat admonitions.

- Information about compat annotations to CONTRIBUTING.md.

- NEWS.md entry for PRs #30090, #30035, #30022, #29978,
  #29969, #29858, #29845, #29754, #29638, #29636, #29615,
  #29600, #29506, #29469, #29316, #29259, #29178, #29153,
  #29033, #28902, #28761, #28745, #28708, #28696, #29997,
  #28790, #29092, #29108, #29782

- Compat annotation for PRs #30090, #30013, #29978,
  #29890, #29858, #29827, #29754, #29679, #29636, #29623,
  #29600, #29440, #29316, #29259, #29178, #29157, #29153,
  #29033, #28902, #28878, #28761, #28708, #28156, #29733,
  #29670, #29997, #28790, #29092, #29108, #29782, #25278

- Documentation for broadcasting CartesianIndices (#30230).
- Documentation for Base.julia_cmd().
- Documentation for colon constructor of CartesianIndices (#29440).
- Documentation for ^(::Matrix, ::Number) and ^(::Number, ::Matrix).

- Run NEWS-update.jl.

Co-authored-by: Morten Piibeleht <morten.piibeleht@gmail.com>
Co-authored-by: Fredrik Ekre <ekrefredrik@gmail.com>

fredrikekre added a commit that referenced this pull request Dec 5, 2018

Addition of NEWS and compat admonitions for important
changes between Julia 1.0 and 1.1, including:

- Custom .css-style for compat admonitions.

- Information about compat annotations to CONTRIBUTING.md.

- NEWS.md entry for PRs #30090, #30035, #30022, #29978,
  #29969, #29858, #29845, #29754, #29638, #29636, #29615,
  #29600, #29506, #29469, #29316, #29259, #29178, #29153,
  #29033, #28902, #28761, #28745, #28708, #28696, #29997,
  #28790, #29092, #29108, #29782

- Compat annotation for PRs #30090, #30013, #29978,
  #29890, #29858, #29827, #29754, #29679, #29636, #29623,
  #29600, #29440, #29316, #29259, #29178, #29157, #29153,
  #29033, #28902, #28878, #28761, #28708, #28156, #29733,
  #29670, #29997, #28790, #29092, #29108, #29782, #25278

- Documentation for broadcasting CartesianIndices (#30230).
- Documentation for Base.julia_cmd().
- Documentation for colon constructor of CartesianIndices (#29440).
- Documentation for ^(::Matrix, ::Number) and ^(::Number, ::Matrix).

- Run NEWS-update.jl.

Co-authored-by: Morten Piibeleht <morten.piibeleht@gmail.com>
Co-authored-by: Fredrik Ekre <ekrefredrik@gmail.com>

fredrikekre added a commit that referenced this pull request Dec 5, 2018

Compat admonitions and NEWS for Julia 1.1 (#30230)
Addition of NEWS and compat admonitions for important changes between Julia 1.0 and 1.1, including:

- Custom .css-style for compat admonitions.

- Information about compat annotations to CONTRIBUTING.md.

- NEWS.md entry for PRs #30090, #30035, #30022, #29978,
  #29969, #29858, #29845, #29754, #29638, #29636, #29615,
  #29600, #29506, #29469, #29316, #29259, #29178, #29153,
  #29033, #28902, #28761, #28745, #28708, #28696, #29997,
  #28790, #29092, #29108, #29782

- Compat annotation for PRs #30090, #30013, #29978,
  #29890, #29858, #29827, #29754, #29679, #29636, #29623,
  #29600, #29440, #29316, #29259, #29178, #29157, #29153,
  #29033, #28902, #28878, #28761, #28708, #28156, #29733,
  #29670, #29997, #28790, #29092, #29108, #29782, #25278

- Documentation for broadcasting CartesianIndices (#30230).
- Documentation for Base.julia_cmd().
- Documentation for colon constructor of CartesianIndices (#29440).
- Documentation for ^(::Matrix, ::Number) and ^(::Number, ::Matrix).

- Run NEWS-update.jl.


Co-authored-by: Morten Piibeleht <morten.piibeleht@gmail.com>
Co-authored-by: Fredrik Ekre <ekrefredrik@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.