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

Deprecate |> #24886

Closed
wants to merge 12 commits into from
Closed

Deprecate |> #24886

wants to merge 12 commits into from

Conversation

bramtayl
Copy link
Contributor

@bramtayl bramtayl commented Dec 2, 2017

No description provided.

@bramtayl
Copy link
Contributor Author

bramtayl commented Dec 2, 2017

Not sure if this is fully sufficient. I think might need to be deprecated to a syntax error if we want non-function-like functionality in the future?

@bramtayl bramtayl changed the title Deprecate |> WIP: Deprecate |> Dec 3, 2017
@shashi
Copy link
Contributor

shashi commented Dec 3, 2017

This has been discussed before, and the consensus was against removing |>. #20331

@bramtayl
Copy link
Contributor Author

bramtayl commented Dec 3, 2017

Recent tide of support here: #5571 (comment)

@StefanKarpinski
Copy link
Sponsor Member

The purpose of deprecating it as a function is to reintroduce it as syntax.

@shashi
Copy link
Contributor

shashi commented Dec 3, 2017

Okay, sounds good.

@yurivish
Copy link
Contributor

yurivish commented Dec 3, 2017

Would reintroducing it as syntax mean that you wouldn't be able to alias it, e.g. say | = |> in order to use a different spelling (say at the REPL)?

Slightly OT, but I wonder if there's a more general mechanism that could be used, e.g. operator macros...

@bramtayl
Copy link
Contributor Author

bramtayl commented Dec 4, 2017

Not sure if the broadcast test i deleted was important...

@bramtayl
Copy link
Contributor Author

bramtayl commented Dec 4, 2017

Worth noting how much uglier code got without chaining (and how much nicer |> x -> expressions could be with syntactic chaining.

@KristofferC
Copy link
Sponsor Member

To me, the code became quite a lot clearer.

@Sacha0
Copy link
Member

Sacha0 commented Dec 4, 2017

Echoing Kristoffer, almost every rewrite in this pull request was clearer to me. Best!

@bramtayl
Copy link
Contributor Author

bramtayl commented Dec 4, 2017

Does anyone know what these deserializing a remote exception errors mean? I probably made a mistake in the triangular and cholmod files but I don't see a clear line number

@bramtayl
Copy link
Contributor Author

bramtayl commented Dec 5, 2017

da da da da da da daaaaaa

@bramtayl bramtayl changed the title WIP: Deprecate |> Deprecate |> Dec 5, 2017
@bramtayl
Copy link
Contributor Author

bramtayl commented Dec 8, 2017

This looks good to me? CI seems unusually happy

@stevengj
Copy link
Member

stevengj commented Dec 9, 2017

I have to say, I don't really understand the reason for deprecating this in order to "turn it into syntax". The proposed syntax in #5571 seems to be mostly about currying, not piping. Currying is useful in lots of cases besides piping, so I don't think currying syntax should be tied to a |> operator at all.

@bramtayl
Copy link
Contributor Author

bramtayl commented Dec 9, 2017

Right now _ is also deprecated, but if I understand #5571 (comment) correctly, it would given meaning only in combination with |>.

1 |>
   _ + 1 |>
   _ + 1

Would be 3.

I guess it would lower to

|>( |>(1, _ -> _ + 1),  _ -> _ + 1)

@bramtayl
Copy link
Contributor Author

bramtayl commented Dec 9, 2017

Here's a particularly horrible line from this PR:

A1 = t1(elty1 == Int ? rand(1:7, n, n) : convert(Matrix{elty1}, (t -> uplo1 == :U ? t : adjoint(t))((t -> chol(t't))(elty1 <: Complex ? complex.(randn(n, n), randn(n, n)) : randn(n, n)))))

Here's how it might be nicer chained:

A1 = 
    randn(n, n) |>
    elty1 <: Complex ? complex.(_, _) : _ |>
    _' * _ |>
    chol |>
    uplo1 == :U ? _ : adjoint(_) |>
    convert(Matrix{elty1}, _) |>
    elty1 == Int ? rand(1:7, n, n) : _ |>
    t1

@bramtayl
Copy link
Contributor Author

bramtayl commented Dec 9, 2017

Unless I'm mistaken, this would be might trickier to do with tight currying alone.

@KristofferC
Copy link
Sponsor Member

That chained stuff is completely unreadable to me.

@bramtayl
Copy link
Contributor Author

bramtayl commented Dec 9, 2017

I guess as someone who's been doing chaining in R for years the unchained alternative is completely unreadable to me.

@Sacha0
Copy link
Member

Sacha0 commented Dec 9, 2017

Both versions are difficult to grok. Better to write such expressions out more clearly if more verbosely. Best!

@stevengj
Copy link
Member

stevengj commented Dec 9, 2017

Giving _ a meaning only in association with |> seems like a huge mistake to me. A terse partial-application syntax for small functions like f(_,y) or _.a is useful in tons of cases that don't involve chaining.

A terse anonymous-function syntax for more complicated cases, just to save writing x ->..., seems less important.

@bramtayl
Copy link
Contributor Author

bramtayl commented Dec 9, 2017

I guess I'd be ok with tight currying too. Vanilla |> and chaining would look like this:

A1 = 
    randn(n, n) |>
    _ -> elty1 <: Complex ? complex.(_, _) : _ |>
    _ -> _' * _ |>
    chol |>
    _ -> uplo1 == :U ? _ : adjoint(_) |>
    convert(Matrix{elty1}, _) |>
    _ -> elty1 == Int ? rand(1:7, n, n) : _ |>
    t1

This is a bit of a mess. The convert line is the only one to take advantage of tight currying. I'm much more of fan of

randn(n, n)
elty1 <: Complex ? complex.(it, it) : it
it' * it
chol(it)
uplo1 == :U ? it : adjoint(it)
convert(Matrix{elty1}, it)
elty1 == Int ? rand(1:7, n, n) : it
t1(it)
A1 = it

ala

#24082

@stevengj
Copy link
Member

stevengj commented Dec 9, 2017

I agree with @Sacha0 that this example is hideous as a single expression of any kind; it seems better to rewrite it entirely, breaking it into multiple lines or even multiple functions.

@bramtayl
Copy link
Contributor Author

bramtayl commented Dec 9, 2017

I'm with you on that. Long chains are definitely confusing and breaking them into shorter chains is usually a good idea.

@stevengj
Copy link
Member

stevengj commented Dec 10, 2017

@bramtayl, it's not so much the length of the chain in this case as all the conditionals. If these conditionals were split into appropriate functions (and probably handled by dispatch, not by runtime checks), it would be a lot more comprehensible (and also might help type inference):

if elty1 == Int   # probably better to handle this check via dispatch too
   A = rand(1:7, n, n)
else
   A = randn(n,n) |> complexify(elty1, _) |> (X -> X'X) |> chol |> fliptri(uplo1, _) |> Matrix{elty1}
end
t1(A)

@bramtayl
Copy link
Contributor Author

bramtayl commented Dec 10, 2017

Yep that looks nice too. No argument from me about splitting out commonly used patterns into functions or that if/else sometimes looks a lot nicer than ?. Even with the simplifications you made, (x -> x'*x) would be nicer as _' * _ unless you wanted to split that out into a function too.

@StefanKarpinski
Copy link
Sponsor Member

When multiple _ are used such as _'_ it is much more common in languages that have these mini closure features to interpret it as (x,y) -> x'y rather than as x -> x'x.

@@ -361,7 +361,6 @@ Julia applies the following order and associativity of operations, from highest
| Bitshifts | `<< >> >>>` | Left |
| Addition | `+ - \| ⊻` | Left[^2] |
| Syntax | `: ..` | Left |
| Syntax | `\|>` | Left |
| Syntax | `<\|` | Right |
Copy link
Member

Choose a reason for hiding this comment

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

If |> is being removed, shouldn't <| also be removed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm. Not sure. Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I see. Yeah dunno.

@bramtayl
Copy link
Contributor Author

bramtayl commented Dec 10, 2017

I guess I've been convinced by @stevengj that _ is more important for use in tight lambdas than for chaining. I think that |> combined with tight lambdas is nice but really only covers a very small subset of what you can use chaining for. If we want proper chaining in Base we will have to use a different symbol. All that we need for proper chaining is a stand-in for the previous evaluation (something other than _) deprecated for 0.1. I'm curious about @oxinabox's thoughts.

@stevengj
Copy link
Member

In cases where a “tight” _ doesn’t work, I’m not convinced that x -> is so onerous.

@bramtayl
Copy link
Contributor Author

If that's the case, then why _ as a lambda at all?

@oxinabox
Copy link
Contributor

I guess I've been convinced by @stevengj that _ is more important for use in tight lambdas than for chaining.

I 100% agree with that. that having terse lambdas is what the _ proposal is about.
It is an end-in-and-of-itself.
Separately from chaining.
The fact that it would make chaining nicer is a side effect.
Its use in map, reduce, and other higher order functions is what is really attractive.

I'm not a huge fan of chaining these days. (It has its place sure)

I think that |> combined with tight lambdas is nice but really only covers a very small subset of what you can use chaining for.

I'm not sure what in particular you are thinking of.

If we want proper chaining in Base we will have to use a different symbol.

Ergo this deprecation, right?

@stevengj
Copy link
Member

stevengj commented Dec 11, 2017

I don't support this deprecation, because I think a _ syntax for short (single-call) anonymous functions combined with x-> for longer anonymous functions is sufficient support for chaining in Julia. (Specialized cases that require a fancier DSL can use macros.) |> seems like it should continue to be an ordinary operator.

I don't like the idea of a special currying syntax tied to |>, as I've explained elsewhere, and supporting such a syntax in the future seems to be the whole reason for this deprecation.

@bramtayl
Copy link
Contributor Author

|> isn't necessary for chaining, so I'd be fine with giving it up to currying.

@bramtayl bramtayl closed this Dec 11, 2017
@StefanKarpinski
Copy link
Sponsor Member

Triage: instead of deprecating, we'll document that |> may become syntax in the future such that using it for pipelining will continue to work as it currently does but overloading it may break.

@oxinabox
Copy link
Contributor

It might be nice to cherry-pick this PR, and keep a lot of the edits to remove |>.
I would say at least half of the code in base cleaner without it.

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.

None yet

9 participants