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

Deprecate array of arrays special casing in .= #24095

Merged
merged 1 commit into from
Oct 20, 2017
Merged

Deprecate array of arrays special casing in .= #24095

merged 1 commit into from
Oct 20, 2017

Conversation

mbauman
Copy link
Member

@mbauman mbauman commented Oct 11, 2017

This commit deprecates the special-casing for arrays of arrays when using A[I...] .= with entirely scalar indices. Once this deprecation goes away, A[1] .= 0 will always modify the element that is stored at A[1]. The outer array A will not be modified, regardless of its element type. I have chosen to use the same internal machinery from @views for this; when the deprecation is removed the syntactic transform will behave like broadcast!(identity, @views A[I...], X).

Edit: Fixes #20158.

@mbauman mbauman added deprecation This change introduces or involves a deprecation broadcast Applying a function over a collection arrays [a, r, r, a, y, s] labels Oct 11, 2017
This commit deprecates the special-casing for arrays of arrays when using `A[I...] .=` with entirely scalar indices.  Once this deprecation goes away, `A[1] .= 0` will *always* modify the element that is stored at `A[1]`.  The outer array `A` will not be modified, regardless of its element type.  I have chosen to use the same internal machinery from `@views` for this; when the deprecation is removed the syntactic transform will behave like `broadcast!(identity, @views A[I...], X)`.

fixup!
Base.@propagate_inbounds dotview(A::AbstractArray, args...) = view(A, args...)
Base.@propagate_inbounds dotview(A::AbstractArray{<:AbstractArray}, args::Integer...) = getindex(A, args...)

Base.@propagate_inbounds dotview(args...) = Base.maybeview(args...)
Copy link
Contributor

Choose a reason for hiding this comment

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

With this dotview wouldn't be needed and could also be deprecated in favor of maybeview, right?

(Though it would require changing this line in the parser)

Copy link
Member Author

Choose a reason for hiding this comment

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

For now the two must be uncoupled in order to have the deprecation... and it's probably safest to keep them separate in general.

@mbauman mbauman merged commit 182d843 into master Oct 20, 2017
@mbauman mbauman deleted the mb/dotview branch October 20, 2017 16:46
Sacha0 pushed a commit to Sacha0/julia that referenced this pull request Oct 22, 2017
This commit deprecates the special-casing for arrays of arrays when using `A[I...] .=` with entirely scalar indices.  Once this deprecation goes away, `A[1] .= 0` will *always* modify the element that is stored at `A[1]`.  The outer array `A` will not be modified, regardless of its element type.  I have chosen to use the same internal machinery from `@views` for this; when the deprecation is removed the syntactic transform will behave like `broadcast!(identity, @views A[I...], X)`.

fixup!
@stevengj
Copy link
Member

Should container types now define dotview or should they define maybeview? See https://discourse.julialang.org/t/broadcasting-difference-between-and-for-dataframes/14955

@mbauman
Copy link
Member Author

mbauman commented Sep 14, 2018

dotview is very specific — it's really only called by the parser on the LHS of these broadcast expressions.

maybeview is far more general — it returns unwrapped elements for scalar indexing and views otherwise.

So if implementing the general behavior will also work for broadcasting, then use maybeview. But in some cases, (e.g., this specialized BitArray broadcasting implementation), we can take advantage of the limited scope of dotview to return something far more specialized.

@stevengj
Copy link
Member

Anyway, this should probably be documented in the manual section on customizing broadcasting for user-defined types...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrays [a, r, r, a, y, s] broadcast Applying a function over a collection deprecation This change introduces or involves a deprecation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants