Conversation
github-actions
bot
added
the
needs version bump
github-actions
bot
removed
the
needs version bump
Codecov Report
@@ Coverage Diff @@
## master #449 +/- ##
==========================================
- Coverage 98.52% 98.48% -0.04%
==========================================
Files 21 21
Lines 2095 2175 +80
==========================================
+ Hits 2064 2142 +78
- Misses 31 33 +2
Continue to review full report at Codecov.
|
src/rulesets/Base/sort.jl
Outdated
| end | ||
|
|
||
| Δxs = InplaceableThunk(@thunk(sort_add!(zero(Δys))), sort_add!) | ||
| Δxs = InplaceableThunk(@thunk(sort_add!(zero(xs))), sort_add!) |
There was a problem hiding this comment.
This seems wrong.
The zero of the primal doesn't have to exist.
(If it does, it is probably also the zero of the differential)
Consider xs::Vector{DateTime}
then Δys is either a Vector{<:Period} or a Vector{<:Tangent{DateTime}}
In this case it is bugged.
JuliaLang/julia#41348
but if it was a a Vector{<:Period} it wouldn't be
oxinabox
left a comment
oxinabox
left a comment
There was a problem hiding this comment.
only 1 comment of any real meaning.
Once addressed merge when ready
| Δfactors isa AbstractZero && return (NoTangent(), Δfactors, NoTangent()) | ||
| factors = F.factors | ||
| ∂factors = eltype(A) <: Real ? real(Δfactors) : Δfactors | ||
| ∂factors = eltypeA <: Real ? real(Δfactors) : Δfactors |
There was a problem hiding this comment.
Check that this works right.
I suspect you need to do
_lu_pullback(ΔF::Tangent, m, n, :Type{eltypeA}, pivot, F) where eltypeA
or julia will not specialize on this and it will result in it being type-unstable.
There was a problem hiding this comment.
It does seem to work right
julia> y, pb = rrule(lu, randn(T, m, n), pivot)
julia> @code_warntype pb(ChainRulesTestUtils.rand_tangent(y))
Variables
#self#::ChainRules.var"#lu_pullback#1571"{Matrix{Float64}, Val{true}, LU{Float64, Matrix{Float64}}, Int64, Int64}
ȳ::Tangent{LU{Float64, Matrix{Float64}}, NamedTuple{(:factors, :ipiv, :info), Tuple{Matrix{Float64}, Vector{NoTangent}, NoTangent}}}
Body::Tuple{NoTangent, Matrix{Float64}, NoTangent}
1 ─ %1 = Core.getfield(#self#, :m)::Int64
│ %2 = Core.getfield(#self#, :n)::Int64
│ %3 = Core.getfield(#self#, :A)::Matrix{Float64}
│ %4 = ChainRules.eltype(%3)::Core.Const(Float64)
│ %5 = Core.getfield(#self#, :pivot)::Core.Const(Val{true}())
│ %6 = Core.getfield(#self#, :F)::LU{Float64, Matrix{Float64}}
│ %7 = ChainRules._lu_pullback(ȳ, %1, %2, %4, %5, %6)::Core.PartialStruct(Tuple{NoTangent, Matrix{Float64}, NoTangent}, Any[Core.Const(NoTangent()), Matrix{Float64}, Core.Const(NoTangent())])
└── return %7but also the @inferred tests should have caught a regression, right?
There was a problem hiding this comment.
nice, good. It constant folds
Yes they should have
3 participants
Closes #408
Needs JuliaDiff/ChainRulesCore.jl#371
to do:
remove examples where the thunks can be unthunked more than once