-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Broadcast had one job (e.g. broadcasting over iterators and generator) #18618
Comments
This is intentional. For example, |
Maybe the problem is that people find the new dot syntax too convenient. There has been desire of having a compact way of expressing |
Also, as @stevengj has pointed out before: there's got to be a difference between |
@stevengj But Iterators do have shape (especially generators) http://docs.julialang.org/en/release-0.5/manual/interfaces/#interfaces I would argue that iterators are in this awkward realm were most things that you would want to do with a container you also want to do with iterators, and yes maybe it is purely the fact that the @pabloferz The main difference between |
Marking which arguments are to be treated as iterable, instead of the function call itself, would remove the ambiguity. I think? |
For |
I ran into this too. Coming from #16769 where I look for a way to |
@rfourquet, you can do |
Yes, but I should have be more precise: I want to use a function which gets the indices as parameters, like |
What would be the drawbacks of broadcasting dimensions of |
@nalimilan, at first glance I think that would be reasonable and probably relatively easy to implement. Would be breaking so should be done by 1.0. |
One potential problem with this is that |
One possibility would be to temporarily (for 1.0) make simple implementation that just copied to an array. That would allow an optimization post 1.0 |
As I said above, I have a PR at #22489 to allow indexing into iterators, if this can help. |
What needs to be done for 1.0 so that at least we can improve the behavior in 1.x? |
Thanks @nalimilan for bringing that up, I wanted to do that as well. If allowing |
👍 Triage recommends making this an error (the safe choice) or calling
|
Do you really suggest treating all scalars as containers by default? That doesn't sound very practical. Looking at how we could either support any iterable, or just throw an error for them until we support them, it looks like we would need a way to identify iterators in |
(Tricky case for future discussion: |
@timholy Since you've done the redesign of |
@JeffBezanson, the whole point of This is why It should be possible to declare a particular type to be a container for |
In an unrelated PR (#25339), @Keno suggested using |
We could have an explicit trait which defaults to |
Jeff's proposal in #18618 (comment) has room to allow for broadcast treating The most compelling part here is that the only sensible definition for a "collection" type is that it is iterable. Yes, that means that strings are collections. Sometimes they are used as such! So let's default to the behavior that easily allows folks to opt-in to the other at the call site. There is a wart here, though. Since numbers are iterable (they even |
EDIT: in an attempt to avoid derailing the thread further, moved to discourse: |
Yeah, my position is just based on the assumption that scalars are a more natural fallback, especially given that collections need to implement some methods (iteration, possibly indexing) while scalars are just "the rest" and have nothing in common. In the end any type will be able to implement any behavior it wants, but we should make it as convenient and logical as possible, which in particular should help avoiding inconsistencies (e.g. some types declared and behaving as scalars and others not). I'm not overly concerned with having a few exceptions for essential types like strings and numbers, as long as the rules are clear for other types. |
I've been thinking a little bit about our iteration and indexing interfaces. I note that we can have useful objects which iterate (but can not be indexed), objects that can be indexed (but are not iterable), and objects that do both. Based on that, I wonder if:
I do also feel that it would be desirable if one-argument (To me, the fact that things like strings may have to be explicitly wrapped up like |
I agree that But being for indexable containers is essentially that containers need a trait to opt-in, which is basically what I've been advocating. |
I'm not sure consecutive indices is a good constraint - dictionaries will have arbitrary indices, for example. However, I don't mind if we make the broadcasting behavior opt-in via trait; I'm just saying that imagining how a generic |
Are people truly happy with this change? I for one have never needed to broadcast over a container without a shape, while I broadcast over things that should be treated as scalars all the time. Every deprecation warning I 'fix' makes me shed a tear. |
What are the types of those things? |
Can be anything. For example, in a package that defines an optimization model type value.(x, model) It's also often the case that the argument types are from other packages, so adding a method to |
Yes, I see your point, and I do agree that it is annoying in situations like that. That said, the old behavior was absolutely problematic — it was one of those "the default fallback is definitively wrong in some cases" things. In short, there are four options that avoid the incorrect fallback:
Option 1 is worse for everyone, option 2 is the status quo, and option 3 is backwards, and option 4 is something we've never done before and likely to be buggy. |
I guess some of the discussion must have happened behind the scenes, but I'm just not convinced by the arguments I've seen in this thread and in #25356 against
This is my main point of disagreement. To me it seems that in all of Julia code I know that having a single method to implement to define iteration is nice, but it's not that much work to implement one more for either a new trait, or for making it required to specify |
That's effectively the design of 0.6, which led to this issue and #26421 and #19577 and JuliaLang/LinearAlgebra.jl#455 and #23746 and possibly more — searching for this is hard. It means that Base is providing a default fallback that is incorrect for a whole class of objects. That's why I prefer a mechanism that errors unless you opt in, one way or another. It's opinionated and the transition is a pain, but it forces you to be explicit. You may be right that there are more "scalar-like" custom types than there are iterator-like ones, but I stand by the fact that broadcasting is first and foremost an operation on containers. I expect And, finally, containers that get the scalar-default treatment simply aren't able to be used elementwise with broadcasting. For example, Those are the main arguments. As far as the ugliness of Ref, I fully agree. A solution there is #27608. |
Fair enough. I don't have any knockdown arguments or magic solutions to those problems, and #27608 will improve things. |
@tkoolen I had the same concerns and use case. @mbauman The arguments given above may not be fully convincing. Here are two questions to be more complete:
Anyway, it might be wise to fix #27563 (e.g. by #27608) and let users play with it a while before 1.0. |
I take it you missed the news that 1.0 has been released. |
@StefanKarpinski Missed that, indeed. Congratulation to all developers, julia is amazing, keep on ! |
It was surprising to find that
broadcast
is not working with iteratorsThis is due to
Broadcast.containertype
returningAny
julia/base/broadcast.jl
Line 31 in 413ed79
leading to the fallback at:
julia/base/broadcast.jl
Line 265 in 413ed79
Defining
containertype
to beArray
for that iterator lead to problems with callingsize
on it, becausebroadcast
doesn't check against the iterator interfaceiteratorsize(IterType)
.map
solves this with the fallbackmap(f, A) = collect(Generator(f,A))
which may be more sensible that the current definition ofbroadcast(f, Any, A) = f(A)
The text was updated successfully, but these errors were encountered: