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

Fold linspace into range, add keyword arguments, deprecate logspace #25896

Merged
merged 4 commits into from
Feb 15, 2018

Conversation

ararslan
Copy link
Member

@ararslan ararslan commented Feb 5, 2018

EDIT: The description below is outdated and reflects the original intent of the PR. Scroll down for current state.


This PR consists of two commits:

  1. Deprecate linspace and its constructed type LinSpace to linrange and LinRange, respectively. These names more accurately reflect what's being constructed.

    It's likely that the name linspace was chosen for Julia since it matches what's used by Matlab. NumPy uses this name as well, but they likely also chose it for Matlab familiarity. In Matlab, the name linspace is short for "linearly spaced," and it returns a vector of linearly spaced elements.

    In Julia, we're often more careful with our terminology: in this case, the returned object is an AbstractRange, not some kind of vector space or any other kind of space. Thus calling it linrange is more indicative of the functionality.

  2. Deprecate logspace to logrange. While the accuracy of terminology argument doesn't apply as strongly to this function as it does for linspace, it still makes sense (at least to me) to make this name match its lin* counterpart.

@ararslan ararslan added deprecation This change introduces or involves a deprecation collections Data structures holding multiple items, e.g. sets labels Feb 5, 2018
@JeffBezanson JeffBezanson added the triage This should be discussed on a triage call label Feb 5, 2018
@timholy
Copy link
Member

timholy commented Feb 5, 2018

I don't deny that linrange is nicer than linspace. But if the motivation is to be precise about types, it's a little weird that logrange won't return a range. As it is, the *space names don't make a "promise" about what kind of object they'll return, but with this change now one of them will be lying.

Unless you want to create an actual LogRange object 😈 ?

@ararslan
Copy link
Member Author

ararslan commented Feb 5, 2018

I can remove the logrange commit, or I can add a LogRange type, whichever is preferable.

@garrison
Copy link
Member

garrison commented Feb 5, 2018

julia> range(1,2,5)
1:2:9

julia> linrange(1,2,5)
1.0:0.25:2.0

Any tips for remembering the difference between range and linrange? I can't imagine I'll be the only one to find this confusing.

@ararslan
Copy link
Member Author

ararslan commented Feb 5, 2018

Oh right, I had forgotten that we still have range...

@JeffBezanson
Copy link
Member

Maybe rename range to steprange?

@ararslan
Copy link
Member Author

ararslan commented Feb 6, 2018

+1 for renaming range to steprange.

Regarding a potential LogRange type, I've been playing around with that a bit, and a difficulty comes in whenever a generic method for AbstractRanges assumes that step is defined. I don't think there's a sensible definition of step(::LogRange), since the step size not constant by definition.

@ViralBShah
Copy link
Member

Good idea to rename range so that it is also available to the user.

@ViralBShah
Copy link
Member

range and linrange also can get confusing.

@jekbradbury
Copy link
Contributor

or range(1, 2, step=0.25) vs range(1, step=2, length=5)?

@nalimilan
Copy link
Member

Regarding a potential LogRange type, I've been playing around with that a bit, and a difficulty comes in whenever a generic method for AbstractRanges assumes that step is defined. I don't think there's a sensible definition of step(::LogRange), since the step size not constant by definition.

That doesn't mean we can't define LogRange <: AbstractArray, does it? It just cannot be an AbstractRange if we consider that step is a required method.

@StefanKarpinski
Copy link
Member

I think having LogRange not be a range is fine, it's the log of a range. The fact that LinSpace has nothing to do with linear spaces and is a kind of range despite the name is what really chaps my hide.

@ararslan
Copy link
Member Author

ararslan commented Feb 7, 2018

Tell you what, I'll remove the logrange commit for now and we can discuss that in a separate PR. That way we can just move forward with the major culprit here: linspace, the liar and deceiver of types.

@StefanKarpinski
Copy link
Member

StefanKarpinski commented Feb 7, 2018

I'd say keep it as is – I don't see what the problem is. Even if it's not a subtype of AbstractRange I think that logrange is still a much better, clearer name than logspace.

@ararslan
Copy link
Member Author

ararslan commented Feb 7, 2018

Okay, I'm fine with that too.

@dpsanders
Copy link
Contributor

Can logrange just be a MappedArray of linrange?

@JeffBezanson
Copy link
Member

More concretely, we could deprecate logspace to e.g. exp10.(range(...)).

@ararslan
Copy link
Member Author

ararslan commented Feb 8, 2018

logspace allows an arbitrary base, so it'd have to be base.^range(...).

@JeffBezanson
Copy link
Member

Triage likes the following plan:

  • deprecate logspace to its definition base.^range(...)
  • deprecate linspace and range to range(start; step, stop, length)
  • after 0.7 we can also allow stop as a second positional argument to range

@ararslan ararslan removed the triage This should be discussed on a triage call label Feb 8, 2018
@JeffBezanson JeffBezanson added this to the 1.0 milestone Feb 8, 2018
@ararslan
Copy link
Member Author

Okay folks. There are now four distinct commits:

  1. Deprecate the positional arguments in range in favor of keyword arguments. Under the hood, the keyword argument method calls an internal function with positional arguments, which has a number of messy methods necessary for avoiding ambiguities. Feedback on how to clean those up and/or consolidate them would be helpful.

  2. Deprecate linspace in favor of range with the stop and length keywords provided and no step, which is then computed automatically as before.

  3. Rename the LinSpace type to LinRange. I kept this type around even after the linspace deprecation to avoid changing the return type for range(a, stop=b, length=n) (née linspace) to some other crazy thing.

  4. Deprecate logspace to its definition.

The tests pass and building documentation works locally, so we'll see what CI has to say about it.

Copy link
Member

@mbauman mbauman left a comment

Choose a reason for hiding this comment

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

Those _range methods are indeed a little messy, but they're really not terrible. It might help to have a comment that describes an overall strategy and organize the methods based upon what's missing, what you're filling in, and what you're converting.

base/range.jl Outdated
"one or the other"))
end
# We know that the passed values are consistent, so `length` is redundant
colon(start, step, stop)
Copy link
Member

Choose a reason for hiding this comment

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

I think it'd be a little safer to discard the step and defer to _range(start::T, ::Nothing, stop::T, len::Integer). Steps are sloppy, lengths and endpoints can and should be exact.

Copy link
Member Author

Choose a reason for hiding this comment

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

stop and length with no step goes through the linspace machinery though. Is that what we want here?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that'd be my preference. unique(diff(r)) likely isn't going to be [step] no matter what we do, but we can make the other three match exactly with a LinRange.

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this case be an error, for over-specifying?

Copy link
Member

Choose a reason for hiding this comment

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

It is an error if !isapprox((stop - start) / (length - 1), step) (five lines up, just outside of the context window GitHub gives you on my comment).

Copy link
Member

Choose a reason for hiding this comment

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

I think it should always be an error to call it this way. The isapprox check is going to be unpredictable, and if you get that error how do you fix your code? Probably by removing one of the arguments.

@gabrielgellner
Copy link
Contributor

Does this final api mean I would convert:
linspace(1, 10, 100)
to
range(1, stop = 10, length = 100)

I like the range name (has basically the same api as R's seq) -- the only thing I feel strange about is the keyword for stop is this just so range can return an infinite iterator? if length is being specified would it be an error to not have a stop?

@JeffBezanson
Copy link
Member

For 0.7 deprecation purposes, yes that is the API. But in 1.0 we can allow range(1, 10, length=100), dropping the need for the stop keyword.

This name more accurately reflects what's being constructed.

It's likely that the name `LinSpace` was chosen for Julia since it
matches what's used by Matlab. NumPy uses this name as well, but they
likely also chose it for Matlab familiarity. In Matlab, the name
`linspace` is short for "linearly spaced," and it returns a vector of
linearly spaced elements.

In Julia, we're often more careful with our terminology: in this case,
the returned object is an `AbstractRange`, not some kind of vector space
or any other kind of space. Thus calling it `LinRange` is more
indicative of the functionality.
@ararslan ararslan changed the title Rename linspace and logspace to linrange and logrange Fold linspace into range, add keyword arguments, deprecate logspace Feb 14, 2018
@ararslan
Copy link
Member Author

CI failures are unrelated.

@ararslan ararslan merged commit 35653ef into master Feb 15, 2018
@ararslan ararslan deleted the aa/linrange branch February 15, 2018 18:44
@stevengj
Copy link
Member

stevengj commented Mar 5, 2018

Important to have Compat support for this. linspace is very widely used.

darwindarak added a commit to darwindarak/PotentialFlow.jl that referenced this pull request Jun 30, 2018
The `linspace` to `range` change is a bit awkward at the moment.
Instead of `linspace(start,stop,len)`, we have to use
`range(start, stop = stop, length = len)`.  The `stop` keyword
is only necessary in v0.7, and will [probably be
relaxed](JuliaLang/julia#25896 (comment))
in v1.0
darwindarak added a commit to darwindarak/PotentialFlow.jl that referenced this pull request Jul 1, 2018
The `linspace` to `range` change is a bit awkward at the moment.
Instead of `linspace(start,stop,len)`, we have to use
`range(start, stop = stop, length = len)`.  The `stop` keyword
is only necessary in v0.7, and will [probably be
relaxed](JuliaLang/julia#25896 (comment))
in v1.0
JeffBezanson added a commit that referenced this pull request Aug 16, 2018
JeffBezanson added a commit that referenced this pull request Oct 18, 2018
JeffBezanson added a commit that referenced this pull request Oct 18, 2018
JeffBezanson added a commit that referenced this pull request Oct 19, 2018
JeffBezanson added a commit that referenced this pull request Oct 19, 2018
JeffBezanson added a commit that referenced this pull request Oct 20, 2018
Bachibouzouk added a commit to Bachibouzouk/Mbo.jl that referenced this pull request Jul 22, 2024
- info replaced by @info
JuliaLang/julia#24490

-linspace has been deprecated in favor of range with stop and length
keyword arguments (#25896).
JuliaLang/julia#25896
Bachibouzouk added a commit to Bachibouzouk/Mbo.jl that referenced this pull request Jul 22, 2024
- info replaced by @info
JuliaLang/julia#24490

-linspace has been deprecated in favor of range with stop and length
keyword arguments (#25896).
JuliaLang/julia#25896
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
collections Data structures holding multiple items, e.g. sets deprecation This change introduces or involves a deprecation
Projects
None yet
Development

Successfully merging this pull request may close these issues.