JuliaLinearAlgebra / LinearMaps.jl

A Julia package for defining and working with linear maps, also known as linear transformations or linear operators acting on vectors. The only requirement for a LinearMap is that it can act on a vector (by multiplication) efficiently.
Other
303 stars 42 forks source link

Flag derivative WRT map itself as not implemented in chain rule #198

Closed gaurav-arya closed 1 year ago

gaurav-arya commented 1 year ago

There are definitely scenarios where there ought to be a gradient WRT some parameters of the linear map, so this PR marks its gradient as not implemented rather than as NoTangent() to try to avoid correctness issues. Here's an example:

function f(p)
           A = LinearMap([p])
           return sum(A * [p])
end
# function is a fancy p^2, so gradient should be 2p

Annoyingly, this doesn't actually fix anything yet, at least for Zygote, because Zygote treats NotImplemented's just like zeros (https://github.com/FluxML/Zygote.jl/issues/1204). But it still seems like the right thing to do.

codecov[bot] commented 1 year ago

Codecov Report

Base: 99.47% // Head: 99.47% // No change to project coverage :thumbsup:

Coverage data is based on head (466f71a) compared to base (50f5bf9). Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #198 +/- ## ======================================= Coverage 99.47% 99.47% ======================================= Files 19 19 Lines 1526 1526 ======================================= Hits 1518 1518 Misses 8 8 ``` | [Impacted Files](https://codecov.io/gh/JuliaLinearAlgebra/LinearMaps.jl/pull/198?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaLinearAlgebra) | Coverage Δ | | |---|---|---| | [src/LinearMaps.jl](https://codecov.io/gh/JuliaLinearAlgebra/LinearMaps.jl/pull/198/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaLinearAlgebra#diff-c3JjL0xpbmVhck1hcHMuamw=) | `100.00% <ø> (ø)` | | | [src/chainrules.jl](https://codecov.io/gh/JuliaLinearAlgebra/LinearMaps.jl/pull/198/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaLinearAlgebra#diff-c3JjL2NoYWlucnVsZXMuamw=) | `100.00% <100.00%> (ø)` | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaLinearAlgebra). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaLinearAlgebra)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

dkarrasch commented 1 year ago

I'm not familiar with the chain rules stuff, but I trust that this is correct. Seems to be closer to the actual fact that the product in itself is differentiable w.r.t. map (in contrast to NoTangent()), but it's just not implemented.