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

Fix broadcast_shape when Base.OneTo is defined #250

Closed
wants to merge 5 commits into from

Conversation

ararslan
Copy link
Member

This provides a workaround for broadcast_shape differences introduced by Base.OneTo in JuliaLang/julia#17137. This is the approach used by DataArrays developed by @tkelman and @timholy.

@@ -1316,4 +1316,9 @@ if !isdefined(Base, :allunique)
export allunique
end

if isdefined(Base, :OneTo)
broadcast_shape(x...) = Base.to_shape(broadcast_shape(x...))
Copy link
Member

Choose a reason for hiding this comment

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

This has to be Base.broadcast_shape because broadcast_shape isn't exported by Base.

If the thought is that people will say using Compat.broadcast_shape, then you also need to add a definition for the case when :OneTo isn't defined.

Maybe add something to the README, too?

@tkelman
Copy link
Contributor

tkelman commented Jul 12, 2016

Did this come up in any other packages?

@ararslan
Copy link
Member Author

NullableArrays

@tkelman
Copy link
Contributor

tkelman commented Jul 12, 2016

Ah, sure if they also need it then I guess we can put it here.

@@ -1256,3 +1256,8 @@ end
let a = rand(10,10)
@test view(a, :, 1) == a[:,1]
end

@test broadcast_shape([1 2; 3 4], [1,1]) == (2,2)
Copy link
Contributor

Choose a reason for hiding this comment

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

Compat.

@davidagold
Copy link
Contributor

Base.OneTo and Base.to_shape are introduced in separate commits --- JuliaLang/julia@2b991c4 and JuliaLang/julia@c318a9f respectively. I guess the odds of this causing an issue are so minuscule that it's not really worth addressing?

@tkelman
Copy link
Contributor

tkelman commented Jul 13, 2016

They were introduced in the same PR so the chances of hitting that on a bisect are small but nonzero. Which was introduced later?

@ararslan
Copy link
Member Author

ararslan commented Jul 13, 2016

Which was introduced later?

JuliaLang/julia@c318a9f

@tkelman
Copy link
Contributor

tkelman commented Jul 13, 2016

check that both are defined I guess?

@davidagold
Copy link
Contributor

Does that mean that if you happen to hit the commit where OneTo is defined and to_shape isn't, then whatever calls broadcast_shape won't work as intended?

@ararslan
Copy link
Member Author

Hm, probably, though I'm not sure what the behavior of broadcast_shape actually was between those commits. Since broadcast_shape isn't exported from base, it probably isn't widely used in code/packages anyway; doing a GitHub code search for Base.Broadcast.broadcast_shape mostly brings up the Julia repo and forks of it.

@tkelman
Copy link
Contributor

tkelman commented Jul 14, 2016

actually this might be backwards. Compat is usually supposed to backport the new behavior to old versions of julia, when it can. the String stuff is kind of an exception because it was especially messy to not break package api's on 0.4.

@timholy
Copy link
Member

timholy commented Jul 14, 2016

It is and it isn't. The argument in favor of using to_shape this way is that the output for broadcast_shape is almost always immediately used to allocate storage, typically an Array. Currently Array is implemented in a form that implicitly assumes indices start with 1. Now, hopefully in Julia 0.6 Array will be entirely rewritten around some MemoryBuffer type, and this from-scratch reimplementation will provide us with a chance---which we may or may not take---to support syntax like JuliaLang/julia#16260 (comment). However, even if we do I very much doubt we're going to force people to allocate them as Array{Float32}(1:3, 1:5) or (even worse) Array{Float32}(OneTo(3), OneTo(5)); in other words, I suspect that we'll permit uses of a Dims{N} tuple as a shorthand for a NTuple{N,OneTo{Int}} for the foreseeable future.

The argument against the automatic use of to_shape is that the result of broadcast_shape is also sometimes used to specify an iteration region. Conceptually, it's much better to write your code so that it preserves the range you're given, and not destroy-and-regenerate as 1:last(rng), because that's much less general than just keeping rng (for those cases where rng does not start with 1). (Note that to_shape only converts to a Dims-tuple when the ranges are all OneTo, so there's nothing "dangerous" about using it, but it is a lost opportunity to migrate code to better constructs.) OneTo is not exactly complicated to deal with once you understand what it means, and it will be good to get packages transitioning from UnitRange->AbstractUnitRange.

Overall, this conversation does make me think that perhaps a better approach is:

  • For Julia 0.5 I should add variants of the Array and BitArray constructors that support a OneTo tuple (obviously still keeping the Int tuple versions)
  • Perhaps we shouldn't merge this after all, on the grounds that people who write packages that exploit the unexported internals of Julia can surely cope with a pretty simple AbstractUnitRange generalization (as long as functions in Base support them appropriately).
  • Perhaps we can make that easier: if anyone can remember/reconstruct the errors that packages using broadcast_shape have gotten due to OneTo, perhaps we can add helpful error messages (e.g., "1:OneTo(n) is not supported, please see devdocs/offset-arrays")

Thanks @tkelman for your perceptive comment.

@andreasnoack
Copy link
Member

@timholy I think we should avoid that the flexible indexing propagates down the tree from AbstractArray to Array. It is nice to have a simple and easy to understand array type so I think the flexible indexing arrays should remain a separate type. Already now, the flexible indexing puts a high tax on writing code for AbstractArray. Many people won't be able to write correct code for AbstractArray.

@timholy
Copy link
Member

timholy commented Jul 14, 2016

For the record, I too would be happy if it stays in wrappers, and we keep Array simple. Whatever we decide there, an even more useful development will be the ability to declare a function as

typealias I1 IndicesStartWith1          # save my poor fingers from too much typing
function foo(A::AbstractArray::I1)
    ...
end

to make it really easy for people to ban arrays with weird indices, yet still support many different AbstractArrays. (Syntax from https://github.com/andyferris/Traitor.jl.) If we had that now, I'd already be helping you sprinkle it throughout your lovely linear algebra code 😄.

@ararslan
Copy link
Member Author

I'm going to close this, as all issues that have resulted from this seem to have since been resolved anyway.

@ararslan ararslan closed this Dec 27, 2016
@ararslan ararslan deleted the aa/oneto branch December 27, 2016 23:20
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.

5 participants