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

Coherent map and comprehension, the imminent death of type_goto #11034

Closed
wants to merge 1 commit into from

Conversation

carnaval
Copy link
Contributor

Let's have this be the basis for discussion. The properties I feel we need are :
(1) map(Y->X,Z) and [X for Y in Z] have the same semantics
(2) We can specify julia's runtime execution without mentions of type inference
(3) The case where isempty(Z) should be the least surprising possible for the user, both from a semantic and performance standpoint

Choosing a type for map(f,T[]) gives us then 3 choices per rule (2) : either Any, Bottom, or T. Choosing T feels completly arbitrary to me, and, as I explained in #7258 (comment), Bottom is quite convenient.
Choosing Any is a performance pitfall which violates rule (3), unless we always return Vector{Any} for not-explicitly-typed maps and comprehensions.

Choosing Bottom has a problem with (3) which is that the empty case is no longer resizable which is "fixed" by making all the cases immutable length : this is what this PR implements.

I then see essentially only two options :
A) this PR
B) the simpler rule of returning only Any arrays except when explicitly asked for a return type (@jakebolewski's prefered solution).

Any opinion on this topic would have to argue one of the following : A) is the way to go, B) is the way to go, one of (1-3) should be broken, my reasoning is incorrect.

@kmsquire
Copy link
Member

I prefer B. I believe the return type should be consistent, and requiring the return type if the output is empty is the best way to guarantee that. (If the user can guarantee that they will never map an empty output, then the current syntax is fine.)

@pao
Copy link
Member

pao commented Apr 27, 2015

Choosing T feels completly arbitrary to me

This statement wasn't obvious to me on the first reading. Having read it a second time, what I think this means is that for a given f, this assumes (using Haskell notation) f:: T -> T? Alternatively, you'd like to handle generically f:: T1 -> T2, but in order to figure out T2 you would need to perform type inference, which violates (2). Is that correct?

@carnaval
Copy link
Contributor Author

@kmsquire B) has the benefit of being simpler to understand, but implies breakage of every single untyped comprehension out in the wild (and in Base). It can even be a silent breakage where you suddenly start boxing every element of a perfectly simple [1.0+x for x in A].

@pao You have it right. It is not that bad given non empty input : what we do now for map which I find good is you assume optimistically that the type of f(first element) represents the return types accurately. If it does then you are on the fast path. If at some point you get input of a different type you reallocate the array and do a copy. The final type is something like Vector{promote_type(f(x[1]), ..., f(x[end]))}. When inference is able to give a tight bound for the return type of f, we can eliminate the checks and get only the fast path.

@kmsquire
Copy link
Member

It can even be a silent breakage where you suddenly start boxing every element of a perfectly simple [1.0+x for x in A].

Currently, the type of this is inferred correctly when the type of A is known. Why would that have to change if we chose B?

@JeffBezanson
Copy link
Member

I think the only viable options are (1) use Bottom for the empty case, (2) throw an error for the empty case and require a type to be specified. While I believe that people have mutated and grown the results of comprehensions, those cases just aren't very important to me. As soon as you start mutating everything, using functional constructs like map makes less and less sense.

@StefanKarpinski
Copy link
Member

There's another option: maps and comprehensions on empty collections without an explicit element type raise an error.

@johnmyleswhite
Copy link
Member

As soon as you start mutating everything, using functional constructs like map makes less and less sense.

I don't think this is a strong position to take. If you're going to replace vectorized functions with map, I don't think you can then say that map is like the removed vectorized functions, except that it adds an additional restriction that the vectorized function didn't have.

@JeffBezanson
Copy link
Member

Vectorized functions have some of the same issues with respect to mutation. Like map, sin(x) also picks the type of result array to return, so you can't arbitrarily mutate its result either. The empty case is the only one that's a bit different.

@carnaval
Copy link
Contributor Author

Raising an error is breaking rule (3) IMO, you can have code which never gets an empty array as input except in some rare case.

@kmsquire Then you are relying on inference, and you will sometimes, randomly, get a Vector{Real} because you, e.g., hit an arbitrary constant limit in inference.jl

@johnmyleswhite You can still write map(sin,Float32,v) but I agree that for people who want to only write sin(v) it is inconvenient

@@ -157,6 +157,7 @@ typedef struct {
unsigned short how:2;
unsigned short isshared:1; // data is shared by multiple Arrays
unsigned short isaligned:1; // data allocated with memalign
unsigned short fixed_len:1; // data cannot be resized (1d only)
Copy link
Member

Choose a reason for hiding this comment

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

Could we set isshared instead, which I think also prevents growing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. I did this quickly mostly to see how much of Base would break. Turns out not too much. I'm not sure how much people would miss dimensions 512-1023 but no reason to waste bits.

Copy link
Member

Choose a reason for hiding this comment

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

Might want to rename the field to resizeable then since it will no longer actually mean shared. The error message can say that the array either has shared data or came from a map or comprehension.

Copy link
Member

Choose a reason for hiding this comment

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

827 is my all-time favorite array dimensionality. Still, I imagine I'll somehow make it through.

@StefanKarpinski
Copy link
Member

@JeffBezanson, I don't get your response – vectorized functions know what the return type should be because the function that's being vectorized is known. sin works just fine on empty arrays:

julia> sin(Float64[])
0-element Array{Float64,1}

The result is correctly typed and mutable. One could argue that it shouldn't be pushed to after that, but it can be and so someone will probably do that. @carnaval's point is that making it always un-pushable, then at least the code either works for all inputs or doesn't work for all inputs and doesn't have this special corner case when the input array happens to be empty.

@JeffBezanson
Copy link
Member

The only mutation-related argument I truly reject is "comprehensions must give Any arrays, because for all you know somebody wants to push in a string and a lemur afterwards". By that standard, sin(x) has the same problem as map. But I agree vectorized functions have the edge in the empty case.

@StefanKarpinski
Copy link
Member

The more I think about this, the more I suspect that this may be the only good option:

  • for type-predictability, empty maps should have element type Union()
  • this implies that at least sometimes the result of a map cannot be resized
  • to be consistent, the result of a map should therefore never be resizeable

This is not really at odds with what @JeffBezanson is arguing – it just enforces the idea that you can't mix and match functional and non-functional programming styles willy-nilly. In particular, you can't push/pop the result of a map. I think this is the only combination that can have good performance and not have corner cases that can unpredictably fail for certain cases (i.e. empty arrays).

@carnaval
Copy link
Contributor Author

@StefanKarpinski This is also my reasoning. The only problem is that it seems it bothers people more when it's about comprehensions and I'd still like uniform semantics for the two operations.

@kmsquire
Copy link
Member

I'm curious what effect this PR would have on conditions in comprehensions (which I'd still like to see):

[x*2 for x in a if isodd(x)]

@carnaval
Copy link
Contributor Author

Other (crazy) solution after talking with @jakebolewski :

Make Array{} always of fixed length. Implement variable length 1d array in julia (wrapper around an underlying Array{}). Find a good name for it (say List for now but it is confusing).
Benefits :

  • Uniformity between 1d and nd cases (n>2)
  • Simplifies the compiler's job
  • Less code in C, more in julia
  • No more map/comprehension problems, except when mapping over a List, which we could define as returning an Array you would have to rewrap explicitely (?)

Cons :

  • Somewhat of a deprecation horror storry

I think doing this we would realize that most code that needs resizeable arrays actually only need a List{Any}, and most numeric code actually needs a fixed size array.

@StefanKarpinski
Copy link
Member

Not entirely crazy. I've considered this as well.

@mbauman
Copy link
Member

mbauman commented Apr 27, 2015

Me too. Another downside is that it adds a non-local pointer dereference that will impact performance due to cache misses and other such low-level craziness. The gains from making the length immutable may offset this, though.

@carnaval
Copy link
Contributor Author

@mbauman But then we could make array types immutable and they would be (with appropriate compiler improvements for non isbits immutables) inlined into the wrapper, so no unneeded dereference.

@mbauman
Copy link
Member

mbauman commented Apr 27, 2015

Sure, but fixed size is different from immutable. We already have wonderful Julian immutable arrays since the tuple re-work.

@carnaval
Copy link
Contributor Author

I meant the array header, with the dimensions, the part the "mutable length" wrapper would have to care for.

@mbauman
Copy link
Member

mbauman commented Apr 27, 2015

Hm, I hadn't considered that. It'd be tricky to get the reference semantics correct, I think, but I've not fully thought it through.

@IainNZ
Copy link
Member

IainNZ commented Apr 27, 2015

I apologize for my ignorance, but is the proposal (and I'm going off the description as expressed by Stefan) that the following code would not be allowed:

x = rand(10)
y = map(z->z^2, x)
push!(y, sum(y))  # Throws some kind of error

Does the typeof of y indicate is inability to change length?

@carnaval
Copy link
Contributor Author

@IainNZ You're right. And no, the type of y would still be Array, which is why the above proposal is more satisfying because the behavior of the value is encoded in its type. They are pretty much incomparable wrt to the level of breaking though (the diff here is quite small, whereas deprecating resize! for Arrays is apocalyptic)

@StefanKarpinski
Copy link
Member

It seems reasonable to me that it's rare to push items onto a vector constructed by mapping or comprehensions. I bet it's less rare to remove items from vectors constructed by mapping, however, and you could in theory allow that – it's only adding items to an empty array that's problematic, after all. You could use two bits and indicate whether the array can be shrunk and/or grown separately. In such a scheme, arrays with shared data would have both bits off, indicating that they can't be resized at all.

@IainNZ
Copy link
Member

IainNZ commented Apr 28, 2015

So if I'm writing a package that has a generic append! type thing, how am I supposed to know that this array x is fixed length and another array y isn't? I feel like it has to be a different type if we go that way.

I admit I don't feel like I have got a good feel for the issues at hand here, but with that caveat aside, it feels like option A is trying to find a "pure"/principled solution to all cases at the expense of a common case. The common case for me is mapping over non-empty vectors, and I like getting a nice normal vector length. I do see how the empty case causes an issue. The understanding I have is that we have the current unfortunate situation:

julia> map(z->0.5z, Int[])
0-element Array{Int64,1}

julia> map(z->0.5z, Int[1,2])
2-element Array{Float64,1}:
 0.5
 1.0

but what I'm not quite getting is why isn't allowing the user to specific the output eltype an option if they care about output type? Either by declaring the output type of the function or by explictly stating the output type. Thats option B right? That feels the most Julian to me - loose and Python-y by default, but optimizable if you are willing to specify it. I feel like its friendly to people who aren't very competent programmers but doesn't really hold back an expert from doing what they want. Please feel free to point out the errors of my ways!

@StefanKarpinski
Copy link
Member

The issue is that it makes code fail unpredictably. It's precisely because you don't usually pass empty arrays to things that having the empty array be such an exception is problematic. Your code seems nice an reliable, it passes all sorts of tests, and you're happy with it. You deploy it and then – wham – in production you end up with an empty array and your code just fails. It's not the end of the world, but it's an unnecessary corner case. Matlab has a huge problem with this because it's full of corner cases that you have to be aware of and defensively program around in order to write reliable code. We don't want to end up in the same situation.

@IainNZ
Copy link
Member

IainNZ commented Apr 28, 2015

What would be the failure mode? Something like this?

function foo(x::Vector{Float64})
  return sum(x)
end
foo(map(z->0.5z, Int[1,2,3]))  #works
foo(map(z->0.5z, Int[]))  # fails due to no method error

@stevengj
Copy link
Member

stevengj commented May 7, 2016

Sorry I'm late to this party, but I'm really concerned that map is type-unstable (the return type is currently Union{Array{Void},Array{someT}}) because of the difficulty of deciding on a type for the empty-array case, and if we add to that comprehensions and broadcast (#4883), then array math in Julia becomes a huge problem for the compiler. This will become even worse if we replace @vectorized functions with a map or broadcast syntactic sugar (#8450), because currently vectorized functions are mostly type stable and now they will become unstable.

Requiring the user to always declare the output type explicitly is not a good solution, because (a) that makes it very difficult to write type-generic code and (b) it precludes any convenient "vectorized" syntax.

I really think we have to use type-inference for the empty case to get a workable solution:

  • map(f, a) etcetera should continue to use the type of f(a[1]) etc. for non-empty a.
  • for empty a, it should use returntype(f, (eltype(a),)), which is evaluated at compile-time if possible (via some intrinsic), defaulting to Any. In the worst case, map will be no less type-stable than now, but in many cases where inference succeeds we will get a type-stable result.
  • For specific functions, it should be possible to overload returntype(f, (T,)) to provide a type declaration to help out inference in tricky cases. Possibly there could be a syntactic sugar for this ala return type declarations #1090.

@timholy
Copy link
Member

timholy commented May 7, 2016

My understanding is that this is viewed as a no-go because it risks a potential conflict between changing internal machinery and the visible behavior of julia. IIUC, inference can have performance consequences but theoretically should not affect the results that your program gives you.

However, I agree that in many places it would be more convenient to be able to exploit julia's introspection capabilities in runtime code. I've wondered whether it would be kosher to write inference_runtime.jl that's a stand-alone implementation of type-inference. inference_runtime.jl would not be used by the compiler, it's just intended for solving problems like this one. AFAICT it could be not much harder than cp inference.jl inference_runtime.jl, adding the latter to sysimg.jl, and then fixing up a few things. Henceforth, though, the two would likely diverge. It would be an extra maintenance burden because inference_runtime.jl would have to track changes in internal representation (it relies on introspection), yet it has to provide users with a guaranteed-stable answer.

@stevengj
Copy link
Member

stevengj commented May 7, 2016

@timholy, realize that I'm only talking about using type inference for the empty-array case. If your program functionality depends on the type of the empty-array case, then None[] is probably not acceptable to you either (because it is a basically useless type).

Furthermore, the alternative is a disaster: once @vectorized functions are replaced with some generic syntax/construct, all vectorized math in Julia will become a source of type instability.

@timholy
Copy link
Member

timholy commented May 7, 2016

I'm not arguing against your position, I'm just communicating my understanding obtained through discussions in #12292.

@johnmyleswhite
Copy link
Member

For specific functions, it should be possible to overload returntype(f, (T,)) to provide a type declaration to help out inference in tricky cases. Possibly there could be a syntactic sugar for this ala #1090.

For Nullable and everything downstream of it, this kind of approach would be a huge win. If you define function application on nullable objects as map(f, x::Nullable), then you very frequently want to run map on an empty container, but you want the output type to be the same type as you'd get with a one-element container.

@carnaval
Copy link
Contributor Author

carnaval commented May 7, 2016

I definitely don't have the last word on this, but as far as my opinion goes, I'm strongly opposed to using the compiler's inference algorithm for this. Julia is a dynamic language, the return type is allowed to depend on arbitrary computations so the problem is by essence undecidable. Since the search tree is unbounded the algorithm cuts it using a bunch of heuristics that are unspecified, more or less involved, and more importantly changing from commit to commit.

other options :

  • (easy one) map is an error when the input is empty and no return type is specified
  • (the saner one IMO) improve codegen for unions. Union{Vector{Int},Vector{Bottom}} should not be such a scary type and we could do a much better job at handling that. I think that inlining the dispatch when there are only two choices would get you most of the way. Do note that the return type of getindex of such a type is still only Int since Bottom is not inhabited by any value.
  • make a completely specified inference algorithm and use that. While possible, a big problem with this is that to make the search bounded, you'll have to pick a set of rules and make them not completely arbitrary

As an example, an easy rule to understand is to drop context sensitivity from the algorithm entirely. Of course that's not what you want since it means that you analyze a method independently from the information at the call site so an argument declared of type Any will be a disaster for precision.

If you do use call site information you're back to the problem of unbounded type depth/tuple length and picking arbitrary limits.

@stevengj
Copy link
Member

stevengj commented May 7, 2016

I'm sympathetic, but the argument is a bit perverse: None[] is a useless and performance-killing result to return for map on empty arrays, but at least it's predictably useless and performance-killing?

Making the behavior more predictable (not depending on inference) doesn't seem like an improvement if it's predictably worse in all cases.

@carnaval
Copy link
Contributor Author

carnaval commented May 7, 2016

None[] is a useless and performance-killing result to return for map on empty arrays, but at least it's predictably useless and performance-killing?

Yes. I agree it is unfortunate. However this is not unpredictable as in "the result is surprising sometimes but I then fix the code and we're good", it's the kind of unpredictability that can come and go with julia versions as well as with all kinds of trivial/tautological refactoring (adding a dead branch, move a common expression to a variable, move a bit of code to a helper function, etc). The fact that this problem is a fundamental one rather than a practical one is what makes me uncomfortable.

On the other hand, if we put the "useless" aside for a moment (I don't have a good solution to that), we could address the performance killing part which is definitely a practical/implementation issue. As a bonus, solving it would probably benefit a lot of julia code that is not strongly typed :-)

Deferring to a future hypothetical compiler optimization is rather unsatisfying for sure, but I'm also quite afraid of baking in inference result in a lot of the standard library and have code rely on that.

@JeffBezanson
Copy link
Member

In the long run I agree with @carnaval here, but in @stevengj's proposal we still get predictable results in the non-empty case, which is 99.9% of cases, so this is still a strict improvement over what we have now. We could revisit if we ever get really good at handling union types.

@carnaval's option 1 (give an error) is also realistic --- if you need an empty array result, you need to specify the element type. This also lets us change behavior in the empty case in the future much more easily.

@vtjnash
Copy link
Member

vtjnash commented May 7, 2016

However, I agree that in many places it would be more convenient to be able to exploit julia's introspection capabilities in runtime code. I've wondered whether it would be kosher to write inference_runtime.jl that's a stand-alone implementation of type-inference

I don't see an issue with that. It doesn't even need to be independent of Core.Inference. It's a serious error to make the compile-time behavior depend on inferred values (esp. generated functions must avoid reflection that could create cycles in the Type domain. Among other issues, I believe that would make dispatch undecidable). But it should be acceptable for the value to be computed at runtime – presumably with compiler support such that it knows it can use :static_typeof or @pure (but be care though, since inference isn't @pure – that would have the same mistake as using the information from a generated function) or some weaker form where the function specifies that the output type is a monotonic subtype of Type).

@JeffBezanson
Copy link
Member

To expand on the nuance in this argument: we already let you do arbitrary computations to pick a type, e.g. rand(Bool) ? Int : String, so there is no reason to prohibit calling any particular code, including type inference itself, when doing such a computation. However, you have to understand how to consume the results, and in particular you need a plan for what to do if inference returns Any. Inference is a suspicious thing to invoke because it is very complicated, and because it appears to be magic but isn't, and the magic could go away at an inconvenient time.

@carnaval
Copy link
Contributor Author

carnaval commented May 7, 2016

@vtjnash I think you would have to keep it separate from the compiler's type inference code since they would not be subject to the same compatibility requirements. We definitely don't want to restrain our ability to change inference results to improve the compiler but changes to the alternate one are breaking since they have an impact on program behavior.

Even with a stable alternate inference, I don't think we could expect the same kind of precision while having it be explainable in a simple way. I'd prefer us not to have to say "well the result of map of empty is basically the thing you want most of the time, to understand the actual rules please read base/inference2.jl". It also feels kind of funny to deploy a this machinery for a case we are all agreeing is not very important.

@stevengj
Copy link
Member

stevengj commented May 7, 2016

How about we throw an error if the result is empty unless one of the following conditions holds:

  • The return type is passed via map(f, eltype::Type, args...) or broadcast(f, eltype::Type, args...)
  • The return type of the function is explicitly provided with the function itself:
    • Initially, this could be provided by overloading returntype(functiontype::Type, argtypes::Type...).
    • Eventually, we could provide syntactic sugar for returntype ala return type declarations #1090.
    • If inference improves or becomes more predictable, we could expand the supported cases further in the future.

This would allow us to still support map and broadcast of empty arrays on the "traditionally" vectorized functions by adding appropriate returntype methods, allowing us to replace @vectorize f with f.(args...) without losing functionality in 0.5.

The danger is that the empty-map case would go untested for a lot of code because it is so rare, but that is still a danger for returning None[] or some other eltype, so we don't lose anything, and an explicit ArgumentError exception is probably easier to track down if something goes wrong (vs. an inscrutable error arising from an unexpected None[] array).

@vtjnash
Copy link
Member

vtjnash commented May 8, 2016

going with returns types (ala #1090) does seem tempting, but the complication I foresee is that you would need to be able to write: map( x -> sin(x), array) and have a means of specifying that returntype(::typeof(#1), T) = returntype(::typeof(sin), T), which seems like it could get cumbersome very fast

well the result of map of empty is basically the thing you want most of the time

true, but the counter-proposal of "the result of map of empty is never the thing you actually wanted" seems hardly much better to me.

@stevengj stevengj mentioned this pull request May 9, 2016
5 tasks
@JeffBezanson
Copy link
Member

Needing to define returntype seems wrong; it duplicates information. I will at least implement #1090.

@stevengj
Copy link
Member

stevengj commented May 10, 2016

@yuyichao's suggestion of a fused-broadcast semantics for f.(args...) in #16285 makes me agree with @vtjnash that handling anonymous functions is too important to punt on.

I still think that using inference for the empty-array case is the least-bad option here. In cases where the caller wants to guarantee the return type in all cases, they can have three options:

  • Use map!/broadcast! and allocate the output array themselves, or use a typed comprehension T[...].
  • Use a returntype declaration of some kind in their function.
  • Explicitly pass the return type, e.g. via broadcast(f, Array{Float64}, args...) or broadcast(f, , args...; T=Array{Float64}). I think it would be better to pass the array type than the eltype here, both because it gives more flexibility and because it meshes nicely with a possible syntactic sugar: f.(args...)::Array{Float64} — this syntax is already supported, but is currently just a typeassert whereas we could potentially pass the type to broadcast.

@StefanKarpinski
Copy link
Member

I still think that using inference for the empty-array case is the least-bad option here.

I have to say that I agree for now, but by 1.0 we should switch to the empty return type being Array{None} and making the compiler support that better. Yes, this will create some incompatibility between versions, but for the current version, it seems like the least bad option.

@jpfr
Copy link

jpfr commented May 25, 2016

going with returns types (ala #1090) does seem tempting, but the complication I foresee is that you would need to be able to write: map( x -> sin(x), array) and have a means of specifying that returntype(::typeof(#1), T) = returntype(::typeof(sin), T), which seems like it could get cumbersome very fast

well the result of map of empty is basically the thing you want most of the time

true, but the counter-proposal of "the result of map of empty is never the thing you actually wanted" seems hardly much better to me.

1.) Assume that return types can be constrained on the level of functions and on the level of methods.
+{T}(x::T, y::T)::T defines that the sum of two instances of T must be T again.
Later definitions may add further refinements that must not contradict prior declarations.
+(x::Int32, y::Int32)::Int64 fail
+{T}(x::T, y::T)::Int64 fail
+(x::Int32, y::Int64)::Int64 ok

If a return type is declared, methods force a promote on the return variable.
The promotion is omitted if the type is known to match.

2.) Allow access to return types during inference
map{T,R}(f::(T::R), Array{T})::Array{R}
When using map, the compiler checks if f at least complies to f::(T::R). (Example: An input type of Union{Int32, Any} instead of Int32 is possible. A return type of Int32 instead of Union{Any, Int32}) is possible as well. The old covariance/contravariance discussion....). If the return "might" be compatible (but only a more broad type like Any can be inferred), a manual promotion to the returned value is included.

The type declaration for map needs to be stated only once. To me, that is not cumbersome at all.

This is not a only good to speed up the language. It also helps developers stating their assumptions that are statically checked.

@JeffBezanson
Copy link
Member

Implemented in 0.5.

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.