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

RFC: Use @: to construct a broadcasted object #31088

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

tkf
Copy link
Member

@tkf tkf commented Feb 16, 2019

This is an experimental patch to allow @: as a macro syntax and then use it to construct a non-materialized broadcasted object. Let's keep the discussion about the syntax in #19198.

@@ -11,7 +11,7 @@ using .Base.Cartesian
using .Base: Indices, OneTo, tail, to_shape, isoperator, promote_typejoin,
_msk_end, unsafe_bitgetindex, bitcache_chunks, bitcache_size, dumpbitcache, unalias
import .Base: copy, copyto!, axes
export broadcast, broadcast!, BroadcastStyle, broadcast_axes, broadcastable, dotview, @__dot__
export broadcast, broadcast!, BroadcastStyle, broadcast_axes, broadcastable, dotview, @__dot__, @:
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe export+document a renamed _lazy as well? Hiding this functionality behind a macro only does not seem right. But the macro is really nice syntax!

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for reviewing the code!

I think it's better to avoid overly increasing the API surface. I don't see what exporting Lazy enables you to do other than what is already possible with @:. Also, implementing Lazy can be done in a few lines so I don't think Base should export it. (Note that it still makes sense for Base to export @:, even though it can be done in a few lines, because it let us use a common syntax across Julia ecosystem.)

Copy link
Contributor

Choose a reason for hiding this comment

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

The main advantage of having a macro is nice looking source.

The cost of macros in general is increased complexity: It is not immediately clear what the AST is, what the types are, etc. Hence, readers of code need to either trust the docs (that don't tell you what they do on the AST), read the macro code, or macro-expand. In this specific case, the macro does not do anything complicated, and could be written as a lazy.(...). I am surely not the only one who tries to use macros sparingly, and I think lazy is important enough that it should be accessible to people who prefer view and dot-syntax or even explicit broadcast over @view, @. and the like.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree on the point that it is ideal to provide non-macro interface for language features whenever appropriate. It certainly makes sense for normal functions. However:

  1. Not every feature in Julia has "normal syntax" equivalent. For example, @inline, @inbounds, @simd, etc. do not have a straight-forward non-macro syntax.

  2. You can always run @macroexpand or Meta.@lower to see what is going on in a given macro (or a specialized syntax). I think macros in Julia are very transparent.

  3. _lazy does very unusual thing in the evaluation of the broadcasting expression (because that's its purpose). There has to be something that signals the readers that the evaluation rule is changed, even if they never used this feature. A macro is the best syntax for signaling this because that's what macros do.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough.

@@ -1211,4 +1211,29 @@ end
end
@inline broadcasted(::S, f, args...) where S<:BroadcastStyle = Broadcasted{S}(f, args)

function _lazy end # only used in `@:` macro; does not have to be callable
Copy link
Contributor

Choose a reason for hiding this comment

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

_lazy <: Function does not seem right.

struct _lazy end and an exported const Lazy = _lazy() with broadcasted(::_lazy, x) = (x,) could be an alternative (users would write either sum(@: a .* b) or sum(Base.Broadcast.instantiate(Lazy.( a.* b)))).

Copy link
Member

Choose a reason for hiding this comment

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

It does hold the place of a function, though. Either which way, it's necessarily a bit of a strange construct, so I'm not sure it really matters.

I don't want to see it exported for exactly that reason. It's odd (but necessary).

"""
macro (:)(ex)
return esc(:($instantiate(($_lazy.($ex))[1])))
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Super cool syntax!

I like the fact that instantiate is called before passing the object on. If a non-macro variant gets exported, I think that should eagerly instantiate as well. That is, @inline broadcasted(::typeof(_lazy), x) = (instantiate(x),) instead of instantiating in the macro? Or am I missing some reason that this cannot work?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea, that's possible. Maybe better to do less in the macro. Another way is to create a custom wrapper object and instantiate in materialize, like you did in the other PR. But I'm seeing this as an implementation detail and it's better to use the simplest one.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unless _lazy gets exported in some form, I agree.

@nalimilan nalimilan added the broadcast Applying a function over a collection label Feb 17, 2019
@@ -1211,4 +1211,29 @@ end
end
@inline broadcasted(::S, f, args...) where S<:BroadcastStyle = Broadcasted{S}(f, args)

function _lazy end # only used in `@:` macro; does not have to be callable
Copy link
Member

Choose a reason for hiding this comment

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

It does hold the place of a function, though. Either which way, it's necessarily a bit of a strange construct, so I'm not sure it really matters.

I don't want to see it exported for exactly that reason. It's odd (but necessary).

@: broadcasting_expression

Construct a non-materialized broadcasted object from a `broadcasting_expression`
and [`instantiate`](@ref) it.
Copy link
Member

Choose a reason for hiding this comment

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

I see this as being akin to a generator; that is, generator : comprehension :: Broadcasted : broadcast. Perhaps that could give us a bit of a better language around this? Maybe call it a broadcast generator?

Copy link
Member Author

@tkf tkf Feb 21, 2019

Choose a reason for hiding this comment

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

I agree that "non-materialized broadcasted object" is a bit too mouthful. It's nice to have a concise word for it.

broadcast generator

Broadcasted is kind of like generators but I'm seeing the generators as a way to define iterators (i.e., limited forms of the coroutines). From this point of view, I find it somewhat confusing. But then Base.Generator is just a lazy version of map in Julia and it's pretty close to Broadcasted. So maybe it's fine? I slightly prefer "lazy broadcast" although the term "lazy" is too overloaded.

generator : comprehension :: Broadcasted : broadcast

By the way, what do : and :: mean here? Maybe Base.Generator corresponds to Broadcasted and [f(x) for x in xs] corresponds to broadcast(f, xs)?

Copy link
Member

@mbauman mbauman Feb 28, 2019

Choose a reason for hiding this comment

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

Those colons aren't code... it's a (now antiquated) way of writing a chained analogy. For example puppy : dog :: kitten : cat can be read "a puppy is to a dog as a kitten is to a cat." Or in other words, the relationship between a generator and a comprehension is comparable to the relationship between Broadcasted and broadcast. I shouldn't have written it like that — someone would only have reason to know that if they specifically took a particular US college entrance exam before 2005. 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, that's an interesting notation. Thanks for using it 😄

@mbauman mbauman added needs tests Unit tests are required for this change needs news A NEWS entry is required for this change triage This should be discussed on a triage call labels Feb 20, 2019
@bramtayl
Copy link
Contributor

Isn't a Generator basically just Broadcasted only containing one item? In which case we can probably merge them.

@mbauman
Copy link
Member

mbauman commented Feb 28, 2019

It could be emulated by a generator with an appropriately constructed iterator and a splat, sure. A zip can easily handle the multiple items, but part we don't have is the definition of the outer shape. Also generators work by iterating, broadcast works by indexing, but again, that could be emulated in the generator's iterable.

The big downside with using a generator is that it loses the indexability, which is a very nice feature. Cf. #31020.

@bramtayl
Copy link
Contributor

Hmm. Well why not just modify Zip to be better at figuring out outer shapes, and modify both to handle indexing appropriately?

@bramtayl
Copy link
Contributor

Or just to clarify, there's really only 2 semantic concepts here: zipping and mapping. So all we really need is a lazy zip and a lazy map in the API. Dispatch should be able to handle the rest under the hood.

@mbauman
Copy link
Member

mbauman commented Feb 28, 2019

What would this gain us? If we want to expose this to users as a generator, then we can simply return (bc[i] for i in eachindex(bc)).

There are only two key questions for this PR:

  • How is it spelled? Is it @:?
  • What API surface does the returned object provide? Iterable? Indexable?

For what it's worth, I'm pretty happy with the current state on both those points.

@tkf
Copy link
Member Author

tkf commented Mar 1, 2019

only 2 semantic concepts here: zipping and mapping

@bramtayl There is also broadcasting, right? I mean, map(Base.splat(+), zip(matrix, vector)) does not work but matrix .+ vector works. (Was it what "outer shape" means?).

@mbauman
Copy link
Member

mbauman commented Mar 1, 2019

Was it what "outer shape" means?

Yes, exactly. We can use Broadcast.Extruded to allow for out-of-bounds indexing in a manner consistent with broadcast, but the zip-like container that's holding onto the Extrudeds needs to know what shape to request in the first place. But this is all totally unrelated to this PR because we already have such an implementation. It's called Broadcasted. And it has lots of extra performance optimizations that a more general implementation would be missing.

@bramtayl
Copy link
Contributor

bramtayl commented Mar 1, 2019

Yup sorry to distract from the PR. Just some things to think about.

@JeffBezanson JeffBezanson added the parser Language parsing and surface syntax label Mar 7, 2019
@JeffBezanson
Copy link
Member

Allowing : as a macro name seems ok to me, but it seems like really obscure syntax to use for this (or for anything, really :-) ).

@tkf
Copy link
Member Author

tkf commented Mar 8, 2019

I commented on why I thought @: was appropriate here: #19198 (comment)

@Keno
Copy link
Member

Keno commented Mar 14, 2019

Triage proposal:

A key observation here is that we could avoid materializing broadcast in iteration contexts, so e.g.

for x in f.(g.(y))
...
end

could just iterate over the broadcasted object directly. That would mean that e.g. (x for x in f.(g.(y))) would be a generator that's equivalent to the Broadcasted object. Shortening this to (for f.(g.(y))) would be reasonable, so e.g. sum(for 1:3.^2) would work for the reduction of the broadcast expression.

@tkf
Copy link
Member Author

tkf commented Mar 15, 2019

This is a very clever observation! (for f.(g.(y))) does sound natural.

Is (for f.(g.(y))) quick to implement for those who know how the parser work? I can have a go at some point (but likely not this or next week). But if someone can just go ahead and hack it please do. Also, I appreciate if someone can give me some pointer where to start (maybe look at how comprehension works?).


By the way, it's not crucial, but regarding:

for x in f.(g.(y))
...
end

could just iterate over the broadcasted object directly.

Are you planning to implement this? I guess it's just a hypothetical feature (as you said could)?

I think this would change the result when the broadcasting includes side-effect like this:

rng = MersenneTwister(0)
acc = 0.0
for x in rand.(Ref(rng)) .+ zeros(3)
    acc *= rand(rng)
    acc += x
end

But, it's irrelevant for the additional syntax (for f.(g.(y))) anyway.

@tkf
Copy link
Member Author

tkf commented Mar 30, 2019

It's implemented in #31553.

@stevengj
Copy link
Member

What if you want both @: and @.? Should we have @.:?

@tkf
Copy link
Member Author

tkf commented Apr 29, 2019

FYI @:@. f(x) already works. But since @JeffBezanson is not supporting @: (#31088 (comment)) I have higher hope for for f.(args...) in #31553.

@oscardssmith
Copy link
Member

Triage thinks this is a good feature with too obscure a name.
Some posible alternatives that were discussed were

  • @lazydot
  • @broadcasted
  • ....... 😆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
broadcast Applying a function over a collection needs news A NEWS entry is required for this change needs tests Unit tests are required for this change parser Language parsing and surface syntax triage This should be discussed on a triage call
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants