Tracker.jl icon indicating copy to clipboard operation
Tracker.jl copied to clipboard

Fix test failures on nightly -- was `vcat`, now `kron`

Open ToucheSir opened this issue 3 years ago • 5 comments

ToucheSir avatar Feb 04 '22 03:02 ToucheSir

Has anyone looked into them? It seems there's a commit by @mcabbott that's linked here?

The vcat issues cause test failures in a bunch of downstream packages, I've seen them e.g. in AbstractDifferentiation, Turing, and DiffRules.

devmotion avatar Aug 20 '22 18:08 devmotion

The linked commit was merged and reverted, https://github.com/FluxML/Tracker.jl/commits/master . I have no memory of what it did or why, though!

mcabbott avatar Aug 21 '22 02:08 mcabbott

There's also an issue with Diagonal after https://github.com/JuliaLang/julia/pull/44615

The old implementation (https://github.com/JuliaLang/julia/blame/b91dd0268a9fad8208284ae394c0dcc863f68048/stdlib/LinearAlgebra/src/diagonal.jl#L80) was using diagm in Matrix(d::Diagonal) so a specific rule for was unnecessary, but with the new impl I'm guessing a rule needs to be defined for either Diagonal to make it TrackedArray or just directly define one for Matrix(d::Diagonal). It's been a while since I've looked at Tracker.jl so uncertain what the best approach is.

torfjelde avatar Aug 21 '22 12:08 torfjelde

Ok, https://github.com/FluxML/Tracker.jl/pull/128/commits/f885295f0c7dceaf6d96b5e8d9847d458c84d057 adds something...

As usual it's a nest of ambiguities, this will make Matrix(Diagonal(param([1,2,3]))) work again, but various combinations of Array{T}(...) have their own methods and I didn't try to cover all of them.

mcabbott avatar Aug 21 '22 15:08 mcabbott

Since this issue is still open, the current failures on nightly involve kron(a::TrackedArray{…,Vector{Float64}}, b::TrackedArray{…,Vector{Float64}}). This is the same problem as https://github.com/JuliaDiff/ChainRules.jl/issues/684.

On Julia <= 1.8, @less kron([1,2], [3,4]) shows it reshaping to call the matrix method, while on 1.9 kron!(rand(4), [1,2], [3,4]) has its own implementation via indexing. It should be sufficient to add a method here which reshapes TrackedVectors.

mcabbott avatar Nov 26 '22 16:11 mcabbott