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

range: Assume start=1 when not given. Use OneTo #39223

Closed
wants to merge 6 commits into from

Conversation

mkitti
Copy link
Contributor

@mkitti mkitti commented Jan 13, 2021

This pull request expands the valid input to range when start can be assumed to be one and allows for a single positional argument to be stop.

Base.OneTo is used if either stop or length is provided as a single argument and are Integers.

julia> range(5)
Base.OneTo(5)

julia> range(6.0)
1.0:1.0:6.0

julia> range(; stop=7.5)
1.0:1.0:7.0

julia> range(; length=8)
Base.OneTo(8)

julia> range(; step=3, stop=7)
1:3:7

julia> range(; step=3, length=7)
1:3:19

julia> range(; stop=10, length=5) # Controversy: assumes step=1 rather than start=1
6:10

This pull request does not enable two or three positional argument syntax.

@antoine-levitt
Copy link
Contributor

I disagree with this as it adds one more caveat to the "three of start, step, stop, length, or two of start, stop, length" rule, and is not particularly useful syntax. I think it's actively good to leave this undefined in case some python people are tempted to use it instead of the more idiomatic 1:n.

@jw3126
Copy link
Contributor

jw3126 commented Jan 13, 2021

At first, I liked, the idea of writing range(10) and getting Base.OneTo(10). But now I think this is a bit confusing.
You write range(stop), then you add some keyword arguments, and suddenly stop becomes start.
I am still fine with methods that don't make the meaning of positional arguments depend on keywords. For instance range(length =10) === Base.OneTo(10) is okay.

@mkitti
Copy link
Contributor Author

mkitti commented Jan 13, 2021

I'm just going along with triage on the single positional argument:
#38750 (comment)

and @StefanKarpinski stop-oriented design here:
#38750 (comment)

If we're going to have a single positional argument range this is it. It also translates well from Python.

base/range.jl Outdated
Comment on lines 58 to 62
* Call `range` with any three of `start`, `step`, `stop`, `length`.
* Call `range` with two of `start`, `stop`, `length`. In this case `step` will be assumed
to be one. If both arguments are Integers, a [`UnitRange`](@ref) will be returned.
* Call `range` with `step` and either `stop` or `length`. `start` will be assumed to be one.
* Call `range` with one of `stop` or `length`. `start` and `step` will be assumed to be one.
Copy link
Sponsor Member

@mbauman mbauman Jan 13, 2021

Choose a reason for hiding this comment

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

Is this accurate and succinct?

Suggested change
* Call `range` with any three of `start`, `step`, `stop`, `length`.
* Call `range` with two of `start`, `stop`, `length`. In this case `step` will be assumed
to be one. If both arguments are Integers, a [`UnitRange`](@ref) will be returned.
* Call `range` with `step` and either `stop` or `length`. `start` will be assumed to be one.
* Call `range` with one of `stop` or `length`. `start` and `step` will be assumed to be one.
* Call `range` with any three of `start`, `step`, `stop`, `length`. The omitted parameter is computed.
* Call `range` with fewer than three parameters, but including `stop` and/or `length`. If not provided, `start` and/or `step` are assumed to be one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is a good question. Can we simplify the documentation?

I'm wary of saying "The omitted parameter is computed" because someone may think additional processing occurs that may take time. In some cases, no additional computation is actually done unless the value of the parameter is actually requested. The preceding paragraph states "Mathematically a range is uniquely determined by any three of start, step, stop and `length". That covers what needs to be said well. I would keep the first bullet point as is.

For the second bullet point,

If stop and length are specified, then only step is assumed to be one. We could have assumed start to be one, but that would have a distinct result. We at least need to say step is assumed to be one first.

How about this:

  • Call range with fewer than three parameters. Either stop or length must be specified. If not provided, step and/or start are assumed to be one in that order of precedence.

@mbauman
Copy link
Sponsor Member

mbauman commented Jan 13, 2021

If stop and length are specified, then only step is assumed to be one. We could have assumed start to be one, but that would have a distinct result. We at least need to say step is assumed to be one first.

Oof, yeah. That means that range(stop=10, length=5) has two reasonable meanings: is it 1:2:10 or is it 6:10?

In my mind, that's enough of an ambiguity to put a nail in this coffin. Sure, we can document an ordering to the defaults, but it feels quite arbitrary.

(I should note that on triage we did discuss this form but only briefly — the focus was on the "improve range" PR.)

@mkitti
Copy link
Contributor Author

mkitti commented Jan 13, 2021

In my mind, that's enough of an ambiguity to put a nail in this coffin. Sure, we can document an ordering to the defaults, but it feels quite arbitrary.

I agree the ambiguity exists with range_stop_length. Perhaps assuming step = 1 there might not even be intuitive without start.

I see a few options:

  1. Document the ambiguity (current status)
  2. Have range_stop_length throw an ArgumentError due to the ambiguity
  3. Restrict the assumption of start = 1 to the one argument case. Keeps the one positional argument syntax. range(; step, stop) and range(; step, length) produces an ArgumentError as before.
  4. Reject this PR in whole

@mkitti
Copy link
Contributor Author

mkitti commented Jan 14, 2021

Closing this in favor of #39241 (only add range(; stop) and range(; length) ) and #39242 (export oneto)

@mkitti mkitti closed this Jan 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants