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

Added support for broadcasted piping .|>. #16

Merged
merged 14 commits into from
Jun 3, 2020
Merged

Conversation

racinmat
Copy link
Contributor

@racinmat racinmat commented May 19, 2020

Fixes #14 .
I added few tests, they are passing, but I'm not entirely sure that it doesn't break anything else.

test/runtests.jl Outdated

# broadcasting
fn(x) = x^2
@test _macroexpand( :(@pipe fn |> _.(1:2) ) ) == :(fn.(1:2))
Copy link
Owner

Choose a reason for hiding this comment

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

what happens if we do:

_macroexpand( :(@pipe fn .|> _.(1:2) ) )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now it produces :(var"._".(1:2)) and @pipe fn .|> _.(1:2) crashes.
I'm not entirely sure, what is desired behaviour, how the resulting code should look like.
In plain Julia fn .|> x->x.(1:2) == fn |> x->x.(1:2), and since fn isa not iterable AFAIK, then fn ,|> produces same result as fn |>, so I'm not sure where I want to go with this.

Copy link
Owner

Choose a reason for hiding this comment

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

we want to match the normal behavour.

Here is an interesting set of cases:

julia> [1,2,2] |> x-> atan.([10,20,30], x)
3-element Array{Float64,1}:
 1.4711276743037347
 1.4711276743037347
 1.5042281630190728

julia> [1,2,2] .|> x-> atan.([10,20,30], x)
3-element Array{Array{Float64,1},1}:
 [1.4711276743037347, 1.5208379310729538, 1.5374753309166493]
 [1.373400766945016, 1.4711276743037347, 1.5042281630190728]
 [1.373400766945016, 1.4711276743037347, 1.5042281630190728]

julia> [1,2,2] .|> x-> atan([10,20,30], x)
ERROR: MethodError: no method matching atan(::Array{Int64,1}, ::Int64)

Copy link
Contributor Author

@racinmat racinmat May 19, 2020

Choose a reason for hiding this comment

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

I see. If I understand it correctly, the embedding of input array is not able to cover cases 2 and 3, because the . notation is not expressive enough, so I should explicitly call broadcast function in the macro, right?
Because I can write test for

@test _macroexpand( :(@pipe [1,2,2] |> atan.([10,20,30], _) ) ) == :(atan.([10,20,30], [1,2,2]))

but I'm kinda lost in decising what should

@test _macroexpand( :(@pipe [1,2,2] .|> atan.([10,20,30], _) ) )

equal to.
What do you think? @oxinabox

@oxinabox
Copy link
Owner

Nice, can we replace every instance of the word elementwise with broadcast they are subtly different (depending how exactly one defined elementwise).

Other than that, and the comment i left re testing extra case.
this looked good to me.

Could you also bump the minor release number in the Project.toml so i can tag a release right after merging this?

@oxinabox oxinabox changed the title Added support for element-wise piping .|>. Added support for broadcasted piping .|>. May 19, 2020
test/runtests.jl Outdated
# broadcasting
fn(x) = x^2
@test _macroexpand( :(@pipe fn |> _.(1:2) ) ) == :(fn.(1:2))
@test _macroexpand( :(@pipe 1:10 .|> _*2 ) ) == :((1:10) .* 2)
Copy link
Owner

Choose a reason for hiding this comment

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

can we also test

[true,false] .|> !
[1, 2] |> .+(_, x)
[1, 2] |>  _ .+ x)
[1, 2] .|> .+(_, x)
[1, 2] .|>  _ .+ x)

@oxinabox
Copy link
Owner

Maybe we can just detect if it is soing to attempt to pipe-broadcast a already broadcasted expression and say:
@pipe does not support broadcasted pipes into already broadcasted expressions. Rather than e.g xs .|> f.(a, _)you can use a normal lambda still:xs .|> b->f.(a, b)`

test/runtests.jl Outdated Show resolved Hide resolved
src/Pipe.jl Outdated

function rewrite_apply(ff, target)
:($ff($target)) #function application
function rewrite_apply(ff, target, broadcast=false)
Copy link
Owner

Choose a reason for hiding this comment

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

can we break this into two functions?
rewrite_apply and rewrite_apply_broadcasted

src/Pipe.jl Outdated
rep
end

function rewrite(ff::Expr, target, broadcast=false)
Copy link
Owner

Choose a reason for hiding this comment

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

can we break this into two functions?
rewrite and rewrite_broadcasted

Copy link
Owner

@oxinabox oxinabox left a comment

Choose a reason for hiding this comment

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

Looks all good to me, one last pair of suggestions then should be good to merge.

@racinmat racinmat requested a review from oxinabox June 3, 2020 12:17
@racinmat
Copy link
Contributor Author

racinmat commented Jun 3, 2020

So, I split them to separate functions, tests are passing, anything else, or is it ready to merge?

@oxinabox
Copy link
Owner

oxinabox commented Jun 3, 2020

LGTM

@oxinabox oxinabox merged commit bc7f09e into oxinabox:master Jun 3, 2020
@oxinabox
Copy link
Owner

oxinabox commented Jun 3, 2020

thanks!

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.

Allow broadcasting
2 participants