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

Define machinery so that Some can be used for broadcast #35778

Closed
wants to merge 11 commits into from

Conversation

MasonProtter
Copy link
Contributor

@MasonProtter MasonProtter commented May 6, 2020

As mentioned in #35675, Some may be a good candidate to use basically like an immutable version of Ref in broadcast expressions. This makes Some a zero dimensional container that knows how to broadcast efficiently.

I figure it might be better to discuss the merits of this idea here rather than clogging up that PR. Another candidate brought up in that thread is to create a new singleton type Only.

I think if we can make Some work for this without breaking anything, it would be preferable to creating yet another singleton type, but I'm wary that there could be hitherto unforeseen negative consequences to adding this behaviour to Some. Perhaps someone is relying on eltype(Some(1)) == Any

Closes #18379

@mbauman mbauman added broadcast Applying a function over a collection triage This should be discussed on a triage call labels May 6, 2020
base/some.jl Outdated Show resolved Hide resolved
@MasonProtter
Copy link
Contributor Author

Is the problem that I'm using some function that's not yet defined? I don't see what's causing the build error.

@mbauman
Copy link
Member

mbauman commented May 6, 2020

Yeah, that's a bootstrap error. It looks like some.jl comes before both multidimensional.jl (so can't use ::CartesianIndex) and broadcast.jl (so you'll have trouble extending Broadcast.broadcastable). I think both those files have some "vacationing" definitions along these same lines (e.g.,

broadcastable(x::Union{AbstractArray,Number,Ref,Tuple,Broadcasted}) = x
and
getindex(b::Ref, ::CartesianIndex{0}) = getindex(b)
)

@MasonProtter
Copy link
Contributor Author

Okay, I split out those new method definitions to a new file and put them after broadcast. We'll see if that helps.

base/somebroadcast.jl Outdated Show resolved Hide resolved
Co-authored-by: Matt Bauman <mbauman@gmail.com>
base/somebroadcast.jl Outdated Show resolved Hide resolved
base/some.jl Outdated
@@ -6,6 +6,8 @@
A wrapper type used in `Union{Some{T}, Nothing}` to distinguish between the absence
of a value ([`nothing`](@ref)) and the presence of a `nothing` value (i.e. `Some(nothing)`).

`Some` is also used in broadcasting to treat it's enclosed value as a scalar.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe more accurate to say

Suggested change
`Some` is also used in broadcasting to treat it's enclosed value as a scalar.
`Some` is also used in broadcasting to broadcast a single value to all axes.

? Though I see that a similar wording is already used in Ref.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel like that suggestion is less likely to be understood. However, maybe not everyone knows the word "scalar" either.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe it's fine as-is if we also explain the actual interface it implements (i.e., it is a 0-dim iterable that supports indexing).

Copy link
Contributor

Choose a reason for hiding this comment

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

This should say its rather than it's in the original formulation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Yuri!

@tkf
Copy link
Member

tkf commented May 6, 2020

We can also replace Ref to Some in:

julia/base/broadcast.jl

Lines 678 to 679 in 3da2c52

broadcastable(x::Union{Symbol,AbstractString,Function,UndefInitializer,Nothing,RoundingMode,Missing,Val,Ptr,Regex,Pair}) = Ref(x)
broadcastable(::Type{T}) where {T} = Ref{Type{T}}(T)

@c42f
Copy link
Member

c42f commented May 7, 2020

This is interesting and promising. I can't say I love Some as a word for "protect something from broadcasting", but I also don't think it's worse than Ref and certainly less ambiguous than Scalar.

Also I think it's very reasonable to have this behavior in Some if we can have a separate and more intuitive name for "protecting a thing from unwrapping". Broadcasting isn't the only case one might want to wrap something to protect it. With that in mind, how about defining

wrap(x) = Some(x)

?

@c42f
Copy link
Member

c42f commented May 7, 2020

Thinking a bit more about this, I feel using Some is unfortunate because while Some has the right semantics, it has a somewhat poor name outside its original use case of wrapping nothing. More logical would be to have Wrapper{T}, and to have functions wrap / unwrap.

Then the obvious question is - what specific cases other than broadcast would benefit from such a completely generic wrapper facility? Would there be utility in user-defined types being able to overload wrap and unwrap?

@MasonProtter
Copy link
Contributor Author

Accidentally deleted the wrong comment, oops. It was something along the lines of

I prefer Some to wrap here, especially because wrap and unwrap are likely to be used out in the wild.

@MasonProtter
Copy link
Contributor Author

There's some discussion related to this on the Zulip right now.

Currently, people are thinking aloud about the possibility of improving eachslice to work on multiple dimensions, which would naturally include the behaviour of Ref.

The idea would be that eachslice(A, dims=(2, 3)) would act like a lazy version of

[view(A, :, i, j, :, ..., :) for i in axes(A, 2), for j in axes(A, 3)]

Consequently, eachslice(A, dims=()) would naturally be a zero dimensional container enclosing A, i.e. eachslice(A, dims=()) would essentially be ImmutableRef(A).

This also opens up some possibilities for syntax sugar. For instance, there were suggestions along the lines of

&(A) == eachslice(A, dims=())

and

A[i, j, &, k] == eachslice(A, dims=(1, 2, 4))[i, j, k]

I'm not totally sure how I feel about this, but I think it's an interesting alternative avenue that could both solve this problem, and improve our eachslice.

@mbauman
Copy link
Member

mbauman commented May 7, 2020

I don't quite follow. The point is to make a non-indexable object work in a broadcast expression by putting it in a 0-dimensional container that has the indexing semantics we need. The extension of that definition of EachSlice there would imply using a view(x) as the 0-dimensional container... but views require indexability of x in the first place. Views always re-index into their parent. For that matter, I think I'd expect eachslice to slice into its argument, too, regardless of its implementation.

@tkf
Copy link
Member

tkf commented May 7, 2020

Would there be utility in user-defined types being able to overload wrap and unwrap?

I don't think this is what we want. You'd need something that is guaranteed to "warp" something. Letting users to overload wrap makes it harder for you to ensure that something is really wrapped at the use-site. I think this is why we should prefer constructor rather than factory.

Consequently, eachslice(A, dims=()) would naturally be a zero dimensional container enclosing A,

eachslice is kind of interesting but I don't think it's super straightforward. For eachslice(A; dims=()) to work, view(A) hence A[] has to be already defined. Maybe we can define view(A) = A so that this is well-defined.

(Edit: It was a race condition 😃. @mbauman already pointed out.)


What we need is the concept of 0-dimensional array but the problem is that there is no common word for this (unlike vector and matrix). Looking at Empty product - Wikipedia, the closest thing I can find is nullary (as [] operator becomes a nullary function). Not sure if it makes sense, though.

@MasonProtter
Copy link
Contributor Author

MasonProtter commented May 7, 2020

I don't quite follow. The point is to make a non-indexable object work in a broadcast expression by putting it in a 0-dimensional container that has the indexing semantics we need. The extension of that definition of EachSlice there would imply using a view(x) as the 0-dimensional container... but views require indexability of x in the first place. Views always re-index into their parent. For that matter, I think I'd expect eachslice to slice into its argument, too, regardless of its implementation.

eachslice(A, dims=()) would not do a view(A). It would involve view(A, :, ..., :) inside a zero dimensional iterable, i.e. ZeroDimContainer(view(A, :, ..., : )).

@tkf
Copy link
Member

tkf commented May 7, 2020

It would involve view(A, :, ..., :)

Ah, you are right. But the main point still holds, right? The input A has to satisfy the array API for this to work.

@MasonProtter
Copy link
Contributor Author

MasonProtter commented May 7, 2020

Yes that's true, though there's nothing stopping us from having EachSlice(A, dims=()) never take a view because it's never necessary to index into it.

To be clear, I don't think there's much point of using eachslice for this unless we had syntax sugar for it that removed the need to actually type eachslice when what you really want is some 0 dimensional immutable container.

@tkf
Copy link
Member

tkf commented May 7, 2020

there's nothing stopping us from having EachSlice(A, dims=()) never take a view

Well, it'd increase the complexity of the implementation, right? If you don't need to special case dims == (), the implementation is much simpler:

eachslice(A; dims) =
    (view(A, f(A, dims, inds)...) for inds in Iterators.product(axes.((A,), dims)...))

function f(A, dims, indx)
    fullinds = ntuple(_ -> :, ndims(A))
    for (i, d) in zip(idx, dims)
        fullinds = setindex(fullinds, i, d)
    end
    return fullinds
end

@MasonProtter
Copy link
Contributor Author

MasonProtter commented May 8, 2020

Well, it'd increase the complexity of the implementation, right?

For an EachIndex that's parameterized on the dims, it'd be simple to just dispatch on the dims being an empty Tuple.

@tkf
Copy link
Member

tkf commented May 8, 2020

That's the special case and complexity I'm talking about. I think a dedicated type is much simpler.

@c42f
Copy link
Member

c42f commented May 11, 2020

Just to record a couple of more speculative options mentioned on Zulip (https://julialang.zulipchat.com/#narrow/stream/137791-general/topic/.60Ref.60.20in.20broadcasting)

  • Box was mentioned as a potential alternative name for the immutable version of Ref. (We'd probably need to rename Core.Box to avoid confusion, but that seems like no great loss.)
  • In a Julia 2.0 future where we had immutable array literals created from [...] syntax, [x] could mean the zero dimensional array of one element (with [x,] being a vector of one element).

@MasonProtter
Copy link
Contributor Author

I think Box is a really nice name for this if we can use it without too much trouble being caused by the existing Core.Box.

@c42f
Copy link
Member

c42f commented May 14, 2020

I think Box is a really nice name for this

Would it make sense for Some to become Box in the future? something(Box(nothing)) would make some kind of sense, and Box seems an appropriately more generic name to have in Base than Some. Some is is great name for wrapping nothing, but a bit of a confusing name to be reused for other purposes.

@tkf
Copy link
Member

tkf commented May 14, 2020

I think ultimately everything is rather arbitrary except 0-dim immutable array literal (or maybe immutable fill(x)).

(Though TBH I'm saying this probably because I just want Some(x)[] === x)

@tkf
Copy link
Member

tkf commented May 14, 2020

Hmm... Can we use const[x] for 0-dim immutable array and const[x,] for immutable vector with one element?

@JeffBezanson
Copy link
Member

I think "box" is too overloaded, e.g. for "boxed values" in languages generally.

@mbauman
Copy link
Member

mbauman commented May 28, 2020

So the big objection to adding functionality to Some is that it's supposed to be well-discriminated from a value that you potentially might have. It also doesn't quite seem to be the best word, and while it's the immutable struct we need, it serves a very different purpose — and that purpose necessitates holding it out so it's not confusable with other values.

Triage was largely in favor of using Fill — a la @dlfivefifty's FillArrays.jl — for this purpose. The 0-dimensional Fill is exactly what we want for broadcast, and linguistically it even dovetails on broadcast's "filling" behavior across the other arguments. The extension to support arbitrary dimensions is so trivial (and so very useful with no overhead!) that it seems to make sense to also include that in Base's definition.

The challenge is that FillArrays.jl is a living and thriving package, and it'll be hard to meaningfully implement the necessary subset that is still functional and capable without some major piracy. It really needs to be defined before base/broadcast.jl in order to provide the default broadcastable for many builtin types, but FillArrays also provides lots of useful LinearAlgebra functionality that'll be hard to define so early. One potential option would be to use a minimal (unexpected) struct for internal builtin use (it's literally 5 LOC), and bring in FillArrays as a stdlib... but we still want to export Fill from Base, so there'd still be some piracy going on and it'd still throw FillArrays into the stdlib tarpit.

Perhaps the best option is simply to define the minimal struct (including the AbstractFill supertype), and expect that FillArrays will pirate (with a seal of approval!) to gain more comprehensive LinearAlgebra functionality.

@dlfivefifty
Copy link
Contributor

I think the “best option” might indeed be the best option: that’s the most settled part of FillArrays.jl, where some of the other design choices (broadcasting, RectDiagonal, Eye) might need some deeper thought before making the cut for StdLib

@JeffBezanson JeffBezanson removed the triage This should be discussed on a triage call label Jul 23, 2020
vtjnash added a commit that referenced this pull request Nov 23, 2021
N5N3 pushed a commit to N5N3/julia that referenced this pull request Nov 30, 2021
This removes the dependence on inlining for performance, so we also
remove `@inline`, since it can harm performance.

make Some type a zero-dim broadcast container (e.g. a scalar)

Replaces JuliaLang#35778
Replaces JuliaLang#39184
Fixes JuliaLang#39151
Refs JuliaLang#35675
Refs JuliaLang#43200
N5N3 pushed a commit to N5N3/julia that referenced this pull request Dec 1, 2021
This removes the dependence on inlining for performance, so we also
remove `@inline`, since it can harm performance.

make Some type a zero-dim broadcast container (e.g. a scalar)

Replaces JuliaLang#35778
Replaces JuliaLang#39184
Fixes JuliaLang#39151
Refs JuliaLang#35675
Refs JuliaLang#43200
N5N3 pushed a commit to N5N3/julia that referenced this pull request Dec 1, 2021
This removes the dependence on inlining for performance, so we also
remove `@inline`, since it can harm performance.

make Some type a zero-dim broadcast container (e.g. a scalar)

Replaces JuliaLang#35778
Replaces JuliaLang#39184
Fixes JuliaLang#39151
Refs JuliaLang#35675
Refs JuliaLang#43200
@MasonProtter MasonProtter deleted the some-broadcast branch October 30, 2024 10:31
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Scalar type for broadcasting
7 participants