improve type stability #255
Conversation
|
bump... |
| f = key -> $op((getproperty(t, key) for t in args)...; kwargs...) | ||
| T = mapreduce(eltype, promote_type, args) | ||
| StructArray{T}(map(f, propertynames(args[1]))) | ||
| StructArray{T}(map((oargs...) -> $op(oargs...; kwargs...), map(components, args)...)) |
There was a problem hiding this comment.
I agree with the change (iterating on a Tuple as it was done before seems incorrect).
This is a little bit hard to read though. Could it be split over several lines giving meaningful names to the variables? Maybe something like
for op in [:cat, :hcat, :vcat]
f = Symbol(:curried, op) # or another descriptive name
@eval begin
function Base.$op(args::StructArray...; kwargs...)
$f(vs...) = $op(vs...; kwargs...)From what I understand named functions are a bit better than anonymous functions here for the purpose of error messages (if the anonymous function throws, the stack trace is a bit harder to read).
|
Left a small comment, but I agree with the change overall. Note that this should also be rebased to include the piracy fix in #254 |
|
Gentle bump, would be good to have this included |
|
Sure, merged and tagged in #258 |
|
Great, thanks! I'm still travelling, didn't have time to update this PR. |
3 participants
Fixes #247 - added tests would fail before.
Thanks @jishnub for help in understanding that issue.
I think the updated code is not only more type-stable, but also cleaner in general.