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: Optional explicit return type for map and broadcast #6547

Closed
wants to merge 4 commits into from
Closed

RFC: Optional explicit return type for map and broadcast #6547

wants to merge 4 commits into from

Conversation

MikeInnes
Copy link
Member

broadcast((x,y)->x+1.5y, (-5:5)', -5:5) => InexactError()
broadcast((x,y)->x+1.5y, Float64, (-5:5)', -5:5) => 11x11 Array{Float64,2}

map(join, ["z", "я"]) => ERROR: invalid ASCII sequence
map(join, UTF8String, ["z", "я"]) => UTF8String["z","я"]

@toivoh
Copy link
Contributor

toivoh commented Apr 16, 2014

It would be good if could make this work for the function returned by broadcast_function as well. Looks good to me otherwise.

@MikeInnes
Copy link
Member Author

@toivoh How's that?

@vtjnash
Copy link
Sponsor Member

vtjnash commented Apr 17, 2014

i feel like this (wanting to add an explicit return type to a function) is a frequent enough pattern that it deserves special syntax, so I opened #6551

@toivoh
Copy link
Contributor

toivoh commented Apr 17, 2014

The usage of broadcast_function is e.g.

const broadcast_f = broadcast_function(f)
...
X = broadcast_f(A, B)

which should give an equivalent result to X = broadcast(f, A, B), but avoid having to look up the generated broadcast function dynamically each time that you want to broadcast f.

As I understand it, this PR allows broadcast(f, T, A, B) but not broadcast_f(T, A, B). I'm not sure who knows how to change the broadcast code to allow the latter right now, though.

Also, shouldn't you use Type instead of DataType? I think the latter precludes things like e.g. union types and tuple types.

@toivoh
Copy link
Contributor

toivoh commented Apr 17, 2014

Another thing to note is that the suggested overloading would be very close to ambiguous in some cases. You can map over tuples, but a tuple can also be a Type.

I agree with @vtjnash wanting to specify the resulting element type is a common pattern, and it would be great to have a common convention for it, but I'm not sure if introducing syntax like in #6551 is worth it. Also, broadcast(f, A, B) :> Array{T} seems a bit clunky to me.

To me, it would seem ideal if we could have a convention for a named argument, like broadcast(f, A, B, T=T). This could be used pretty widely, like e.g. +(A,B,T=T). But I don't know enough about the current and possible efficiency of named arguments to say whether this would be reasonable.

@MikeInnes
Copy link
Member Author

This actually works with no changes, since broadcast_f simply delegates to the generic broadcast function.

const broadcast_f = broadcast_function(x->x^2)
broadcast_f([1,2,3]) => [1, 4, 9]
broadcast_f(Float64, [1,2,3]) => [1.0, 4.0, 9.0]

(you're right about using Type though, that's an oversight)

Mapping over tuples of types definitely seems like an uncommon edge case. A keyword argument would do the job for map, but they aren't supported by anonymous functions (yet). Personally I'd be happy to leave it as a caveat that if you want to do this, you just have to specify return type, but another option would be to have a mapT variant which explicitly invokes the right method for tuples.

@toivoh
Copy link
Contributor

toivoh commented Apr 17, 2014

Hmm, that seems to defeat the whole purpose of broadcast_function. I didn't realize that broadcast_function was currently returning an anonymous function. But if it is fixed to return the generated function instead, it can be made to work then.

@MikeInnes
Copy link
Member Author

I'm not sure broadcast_function could be implemented other than with a closure – how are you expecting it to behave? As far as I can tell it's only intended as a convenience.

Perhaps problems with broadcast's current implementation should be discussed in a separate issue, to avoid conflating them with this change.

Thinking about it, named arguments won't work: this version of map should be a seperate method so that the compiler can take advantage of its type stability. map(f, T, x) seems to be the only viable option.

@MikeInnes MikeInnes changed the title Optional explicit return type for map and broadcast RFC: Optional explicit return type for map and broadcast May 19, 2014
@jiahao jiahao force-pushed the master branch 3 times, most recently from 6c7c7e3 to 1a4c02f Compare October 11, 2014 22:06
@MikeInnes
Copy link
Member Author

Looks like there's no interest in this (and/or other approaches are in the works) so I'll close.

@MikeInnes MikeInnes closed this Oct 23, 2014
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.

3 participants