Use return types of calculations instead of promote_type

[devmotion]

This (or at least the last line in the diff) seems to fix the MWE in #252. I'd also argue that it is more correct since, e.g. for Bool promote_type(Bool, Bool) === Bool but true + false isa Int.

I'm not sure what tests could be added to avoid AD regressions in the future. Maybe the boolean examples would be a start as they would have caught the incorrect types at least?

Fixes #252.

[codecov]

Codecov Report

Merging #273 (954df4a) into master (e3c8eb9) will increase coverage by 0.12%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #273      +/-   ##
==========================================
+ Coverage   99.76%   99.88%   +0.12%     
==========================================
  Files           5        5              
  Lines         836      857      +21     
==========================================
+ Hits          834      856      +22     
+ Misses          2        1       -1     

Impacted Files	Coverage Δ
src/FillArrays.jl	99.73% <100.00%> (+0.29%)	⬆️
src/fillalgebra.jl	100.00% <100.00%> (ø)
src/fillbroadcast.jl	100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[jishnub]

Could you add some tests for this?

[devmotion]

I added a few tests in the last commit, but I plan to extend them a bit more.

The PR seems to fix the MWE in its current form but I still have to rerun the DistributionsAD tests. To summarize:

• Instead of promote_type the output types are computed based on the result of the desired calculation
• Instead of type parameters the calculations are based on eltype - given that the original issue was an AD problem and for quite a few of these tracked AD array types the type parameter and eltype are different, this seemed safer
• Simplified the output of broadcasted(::typeof(+)/typeof(-), ::AbstractVector, ::ZerosVector): If the vector already has the correct element type, it is returned directly instead of a Broadcasted expression.
• Removed special broadcasted definition for AbstractRange (covered already by AbstractVector)
• broadcasted(TT, a) is changed to broadcasted(x -> TT(x), a) since type (constructors) are known to cause problems and performance regressions with different AD backends
• Added an optimized broadcasted for ::ZerosVector .- ::AbstractVector
• Implemented broadcasted(::typeof(+), ::ZerosVector, ::AbstractVector) separately without falling back to broadcast(::typeof(+), ::AbstractVector, ::ZerosVector) to avoid commutativity assumption

With these fixes, optimizations, and simplifications _copy_oftype is not used anymore. Therefore I removed it - but unfortunately it seems ArrayLayouts uses this internal function. I'll see if I can open a downstream PR (possibly one can just copy the definition to ArrayLayouts?).

[devmotion]

I added more tests that cover the faulty Bool behaviour on the master branch.

I also checked locally that together with JuliaLinearAlgebra/ArrayLayouts.jl#148 this PR fixes the test failures in DistributionsAD. I also opened TuringLang/DistributionsAD.jl#250 to be sure that the PRs fix our CI tests.

Edit: DistributionsAD CI passes 🙂

[devmotion]

@jishnub Downstream tests pass now 🙂

[dlfivefifty]

Can you add some @inferred tests? It's not 100% clear to me this change is type-stable

[devmotion]

I added a few @inferred, I don't see any possibility for new type instabilities so I assume tests will pass.

[dlfivefifty]

Looks like the inference tests are failing. Are these regressions?

[devmotion]

Seems Julia still has more type inference issues with closures than I had anticipated 👀 The last commit fixes them.

[devmotion]

Can you make a new release with this PR such that we can use this bugfix in Turing etc?