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

Turn off struct broadcast by default. #260

Merged
merged 5 commits into from
Feb 28, 2023
Merged

Conversation

N5N3
Copy link
Contributor

@N5N3 N5N3 commented Dec 12, 2022

This PR limits the struct broadcast to supported array types. (Array, GPUArray...)
Unsupported array types (SparseArray) now do broadcast by ignoring the StructArray wrapper.
close #259

@N5N3
Copy link
Contributor Author

N5N3 commented Feb 21, 2023

@piever kindly bump.

It would be good to merge this before working on #263

@@ -38,5 +38,6 @@ function GPUArraysCore.backend(::Type{T}) where {T<:StructArray}
isconsistent || throw(ArgumentError("all component arrays must have the same GPU backend"))
return backend
end
_use_default_bc(::GPUArraysCore.AbstractGPUArrayStyle) = true
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this could have a full, explicit name and a docstring, as packages that create custom arrays might wish to overload it. Should this be called use_structarray_broadcast instead? If I understand correctly, we are asking whether to use the StructArrays broadcast machinery or not.

Copy link
Contributor Author

@N5N3 N5N3 Feb 24, 2023

Choose a reason for hiding this comment

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

It actually asked if the BroadcastStyle fallbacks to the copy method defined in Base.Broadcast.
Perhaps renamed as has_copy_overloaded?
Edit: My fault, the broadcast overloading is tricky thus perhaps always_structarray_broadcast is better.
I'll add what kind of style could should return true in doc string.

src/structarray.jl Show resolved Hide resolved
@piever
Copy link
Collaborator

piever commented Feb 24, 2023

Sorry for the very late review, I've been swamped lately! It mostly looks good, I've left a couple of small comments.

This makes sure `Tuple`'s axes static during struct static broadcast.
src/structarray.jl Outdated Show resolved Hide resolved
src/structarray.jl Outdated Show resolved Hide resolved
@piever
Copy link
Collaborator

piever commented Feb 28, 2023

LGTM! I've made some small wording changes, if you are happy with those I can just accept them and merge.

@N5N3
Copy link
Contributor Author

N5N3 commented Feb 28, 2023

LGTM!

@piever piever merged commit 569c70e into JuliaArrays:master Feb 28, 2023
@N5N3 N5N3 deleted the bc-fix branch March 1, 2023 00:53
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.

Breakage in sparse structarray broadcasting
2 participants