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

add Returns #39794

Merged
merged 17 commits into from
Apr 1, 2021
Merged

add Returns #39794

merged 17 commits into from
Apr 1, 2021

Conversation

jw3126
Copy link
Contributor

@jw3126 jw3126 commented Feb 23, 2021

This PR adds Always Returns which represents "constant" functions. So instead of writing

f = (args...; kw...) -> 42

you can do f = Returns(42).
Benefits are:

  • Possibility to dispatch on Returns:
mymap(f, x::Array) = ...
mymap(f::Returns, x::Array) = FillArrays.Fill(f.value, size(x))
  • Easier to debug:
Stacktrace:
 [1] filter(Returns(42), a::UnitRange{Int64})

instead of

Stacktrace:
 [1] filter(f::var"#1#2", a::UnitRange{Int64})

@rfourquet
Copy link
Member

Always sounds like a duration belonging to the Dates module :)

@KristofferC KristofferC added triage This should be discussed on a triage call feature Indicates new feature / enhancement requests labels Feb 23, 2021
@mbauman
Copy link
Member

mbauman commented Feb 23, 2021

Are there any cases where this is helpful in Base code itself?

@vtjnash
Copy link
Member

vtjnash commented Feb 23, 2021

Would it be too much of a pun to define this using (c::Core.Const)(x...; y...) = c.val? 😂

@jw3126
Copy link
Contributor Author

jw3126 commented Feb 24, 2021

Are there any cases where this is helpful in Base code itself?

I think so. A pattern which I personally use often is ntuple(_ -> 3, len) and I also found this in julia. e.g.

trailing5 = CartesianIndex(ntuple(x->1, max(ndims(B)-5, 0)))

Another one is _ -> true

@test findprev(x->true, b1, 5) == 5

So I estimate, there is probably > 100 places in julia where Always could be used. Mainly in tests, but some also in Base code. I will convert a couple of them now and if triage is in favor of this, I will hunt all of them down. Or maybe 80% of them.

@jw3126
Copy link
Contributor Author

jw3126 commented Feb 24, 2021

Always sounds like a duration belonging to the Dates module :)

We can use another name of course. Names that came to my mind were Always, Const, Constant, Return. Looked at this naming problem in isolation Const would be my first choice. But as @vtjnash pointed out it collides a bit with Core.Const.

@JeffBezanson
Copy link
Member

Common lisp calls this constantly.

base/abstractarray.jl Outdated Show resolved Hide resolved
@JeffBezanson
Copy link
Member

Suggestion from triage: call it Returns(val), which should make it very clear that it's a function that ... returns the given value.

@jw3126
Copy link
Contributor Author

jw3126 commented Feb 26, 2021

Suggestion from triage: call it Returns(val), which should make it very clear that it's a function that ... returns the given value.

Sounds good. So this means triage is in favor of adding this functionality?

base/operators.jl Outdated Show resolved Hide resolved
base/operators.jl Outdated Show resolved Hide resolved
@JeffBezanson JeffBezanson added needs news A NEWS entry is required for this change and removed triage This should be discussed on a triage call labels Mar 1, 2021
@jw3126
Copy link
Contributor Author

jw3126 commented Mar 4, 2021

I changed most of the constant lambdas in Base and test into Returns. From my point of view, this PR is ready.

@jw3126 jw3126 requested a review from JeffBezanson March 9, 2021 08:16
@jw3126
Copy link
Contributor Author

jw3126 commented Mar 15, 2021

bump

@simeonschaub simeonschaub changed the title add Always add Returns Mar 15, 2021
@simeonschaub simeonschaub removed the needs news A NEWS entry is required for this change label Mar 15, 2021
NEWS.md Outdated Show resolved Hide resolved
test/arrayops.jl Outdated Show resolved Hide resolved
jw3126 and others added 2 commits March 15, 2021 23:33
Co-authored-by: Simeon Schaub <simeondavidschaub99@gmail.com>
@jw3126
Copy link
Contributor Author

jw3126 commented Mar 21, 2021

Bunp

@rfourquet
Copy link
Member

Was Return mentioned as an alternative to Returns ? Verbs are generally used in imperative form, like fill/Fill, map, filter, Iterators.[tT]ake, etc.

@jw3126
Copy link
Contributor Author

jw3126 commented Mar 21, 2021

Was Return mentioned as an alternative to Returns ? Verbs are generally used in imperative form, like fill/Fill, map, filter, Iterators.[tT]ake, etc.

It was mentioned above but not discussed further I think.

@jw3126
Copy link
Contributor Author

jw3126 commented Mar 26, 2021

So how to move forward from here? Should this be renamed from Returns to Return?

@MasonProtter
Copy link
Contributor

MasonProtter commented Mar 29, 2021

Return was discussed on the triage call, but people felt it was too different a concept from the return keyword. Returns was felt to be more descriptive and easier to differentiate.

@JeffBezanson JeffBezanson merged commit b2a586e into JuliaLang:master Apr 1, 2021
@StefanKarpinski
Copy link
Member

I'm amused by how every usage of this made the code longer. I suppose the advantage is that Returns(val) is only compiled once per argument type instead of having a different function object each type that's compiled separately.

@jw3126
Copy link
Contributor Author

jw3126 commented Apr 6, 2021

I'm amused by how every usage of this made the code longer. I suppose the advantage is that Returns(val) is only compiled once per argument type instead of having a different function object each type that's compiled separately.

@StefanKarpinski in general the main selling points of Returns are better printing, the possibility to dispatch on it and less work for the compiler in case of repetitions.
I think in most of the replacements in this PR, none of these are super strong arguments. It was more about consistency for me.

@mbauman
Copy link
Member

mbauman commented Apr 6, 2021

I too was skeptical about this initially, but then immediately found myself wanting to use it for DataStructure.jl's DefaultDict, which calls Callable default values upon access. DefaultDict(Returns(identity)) is more readable and obvious than DefaultDict(x->identity) which feels odd to me.

@kmsquire
Copy link
Member

more readable and obvious than DefaultDict(x->identity) which feels odd to me.

That does seem odd—are you storing a function in the dictionary (certainly possible), or do you mean DefaultDict(identity)?

@mbauman
Copy link
Member

mbauman commented Apr 10, 2021

Yes, I was storing a transformer function in a dict. Type stability is overrated. :)

@JeffBezanson
Copy link
Member

This also has an advantage when the value might be expensive to compute, e.g. Returns(expensive()) instead of val = expensive(); x->val. Admittedly that case is probably rare.

ElOceanografo pushed a commit to ElOceanografo/julia that referenced this pull request May 4, 2021
antoine-levitt pushed a commit to antoine-levitt/julia that referenced this pull request May 9, 2021
johanmon pushed a commit to johanmon/julia that referenced this pull request Jul 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Indicates new feature / enhancement requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants