Infer outputtype by Base.return_types #562
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #562 +/- ##
==========================================
+ Coverage 67.34% 68.55% +1.21%
==========================================
Files 15 15
Lines 2205 2204 -1
==========================================
+ Hits 1485 1511 +26
+ Misses 720 693 -27
🚀 New features to boost your workflow:
|
|
I don't understand why this change is not hit by codecov |
|
I found it, we didn't include the broadcast tests. They are now enabled. |
src/DAT/broadcast.jl
Outdated
| intypes = (eltype.(args2)...,) | ||
| @debug intypes | ||
| outtypes = Base.return_types(bc.f, intypes) | ||
| outtype = Union{outtypes...} |
There was a problem hiding this comment.
this doesn't do unions of Float64, Float32 and such, right? I guess promotion applies there. Or things like Union{Float64, Float64}, because of the splat.
There was a problem hiding this comment.
Ah yes, you are right we should be doing a promote_type instead of the Union to merge Float32 and Float64 to return Float64
There was a problem hiding this comment.
| outtype = Union{outtypes...} | |
| outtype = promote_type{outtypes...} |
| @test isa(a .+ b, YAXArray) | ||
| end | ||
|
|
||
| @testset "missing handling" begin |
There was a problem hiding this comment.
could we also test the promote_type? as well as forcing the outtype? just to make sure things work in all cases.
There was a problem hiding this comment.
Yes we could also test the promote_type.
How would you force the outtype in a broadcast operation?
What would be the syntax?
There was a problem hiding this comment.
I agree outtype does not exist for broadcast but only for xmap. However, a quick test for something a function like f(x) = rand() > 0.5 ? Float64(1) : Float32(1) and then f.(cube) would be nice to add.
There was a problem hiding this comment.
Is this coming from an actual use case or is this good to have in case?
For Union{Float32, Float64} we run into problems with zero because this is not defined for these general unions. zero is used in DiskArrayEngine.create_userfunction to provide an init value.
The Union{Missing, T} works, because this is special cased for zero and one in Julia Base.
| args2 = map(to_yax, args2) | ||
| # determine output type by calling `eltype` on a dummy function call | ||
| dummy_args = map(a -> first(a.data), args2) | ||
| outtype = typeof(bc.f(dummy_args...)) |
There was a problem hiding this comment.
applying f here is for a reason, isn't? e.g. this could determine if the output is just a number or an Array, an f that reduces along a given dimension or not, like cumsum.
not sure...
|
@felixcremer ? merge? or do we still want?
|
|
is this related to #540 ? |
No, this was a user error on my part and we could ease this a bit with a better error message. |
This should make broadcasting mixed type arrays much more robust.
I had the problem that when I broadcasted two YAXArrays with Union{Missing, UInt8} the outtype was determined as Missing and then the actual results where not able to be written into the Array.
I added a test for such a case.
I am wondering whether we should also make this the default for xmap outtypes in general.