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

Use only safe axis types for Broadcast.combine_axes #30164

Merged
merged 1 commit into from
Nov 30, 2018

Conversation

timholy
Copy link
Sponsor Member

@timholy timholy commented Nov 27, 2018

#30074 used the wrong notion of consistency since OneTo(1) is consistent (wrt broadcasting) with any range, but OneTo cannot handle Slice(-1:1).

I also changed the name to something a bit more "override-worthy." I don't think anyone should ever need to do so (the BroadcastStyle is in charge of output container type), but given that we allow axes to determine output from similar it seemed safer to set this up in a way that isn't inconsistent with potential extensions.

#30074 used the wrong notion of consistency since `OneTo(1)` is
consistent (wrt broadcasting) with any range, but `OneTo` cannot
handle `Slice(-1:1)`.
@vtjnash
Copy link
Sponsor Member

vtjnash commented Nov 27, 2018

Is this really just promote?

@timholy
Copy link
Sponsor Member Author

timholy commented Nov 27, 2018

Is this really just promote?

It's effectively

promote_rule(::Type{T1}, ::Type{T2}) where {T1<:AbstractUnitRange,T2<:AbstractUnitRange} = UnitRange{Int}

but limited to broadcasting (without the consequent fallout elsewhere). Since BroadcastStyle should resolve the container type, we can aggressively coerce the axes to a common type.

Of course one might wonder whether we want that rule anyway. This is being conservative and limiting the domain of applicability.

@timholy
Copy link
Sponsor Member Author

timholy commented Nov 27, 2018

To explain the complexities a bit more, consider the following:

Object Values axes(_, 1)
-1:1 -1:1 1:3
OneTo(3) 1:3 1:3
Slice(-1:1) -1:1 -1:1

The Values column is what the object would compare to using for (a, b) in zip(A, B) @test a == b end (i.e., disregarding the axes). None of these should compare as == to one another, since that should require that the values and the axes both match (but see "bugs" below).

These objects raise interesting questions with regards to how construction and convert should behave: one nice "definition" of convert was given by @dlfivefifty here, and by that logic convert should fail for most of the object/type pairs here (specifically for -1:1 to Slice or OneTo, and Slice(-1:1) to UnitRange or OneTo). Since promote gets resolved to a call to convert, these aren't covered by promotion rules.

Bugs noticed while writing this post

Current behavior:

julia> -1:1 == Base.Slice(-1:1)
true

but

julia> using OffsetArrays

julia> collect(-1:1) == OffsetArray(collect(-1:1), (-2,))
false

This, I think, has to be fixed.

More questionable is whether convert(Base.Slice, -1:1) should fail. By the logic above it should. ; if so, I took one line of this PR on the wrong direction, and should have stayed with the constructor syntax. I can fix that before this merges.

@StefanKarpinski
Copy link
Sponsor Member

cc @mbauman

@kshyatt kshyatt added the broadcast Applying a function over a collection label Nov 28, 2018
@timholy timholy merged commit 795591f into master Nov 30, 2018
@timholy timholy deleted the teh/bcast_axis_promotion branch November 30, 2018 09:05
@KristofferC KristofferC mentioned this pull request Dec 30, 2018
53 tasks
@StefanKarpinski StefanKarpinski added triage This should be discussed on a triage call backport 1.0 and removed triage This should be discussed on a triage call labels Jan 31, 2019
@JeffBezanson JeffBezanson removed triage This should be discussed on a triage call triage backport pending 1.0 labels Jan 31, 2019
@KristofferC KristofferC mentioned this pull request Feb 4, 2019
39 tasks
@KristofferC KristofferC mentioned this pull request Aug 26, 2019
55 tasks
@KristofferC
Copy link
Sponsor Member

Breaks a package on 1.0 so removing from backport.

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.

6 participants