Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Diagonal in Array types #231

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

tmigot
Copy link

@tmigot tmigot commented May 16, 2023

Close #223

@codecov
Copy link

codecov bot commented May 16, 2023

Codecov Report

Patch coverage has no change and project coverage change: +3.29 🎉

Comparison is base (65cd309) 81.46% compared to head (ec92e57) 84.76%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #231      +/-   ##
==========================================
+ Coverage   81.46%   84.76%   +3.29%     
==========================================
  Files          18       18              
  Lines        1581     1930     +349     
==========================================
+ Hits         1288     1636     +348     
- Misses        293      294       +1     
Impacted Files Coverage Δ
src/ReverseDiff.jl 100.00% <ø> (ø)

... and 17 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@devmotion
Copy link
Member

I think, even though this PR might fix the issue (untested it seems) it's not the fix we want: The package already defines (too) many methods (see eg. #226) and adding Diagonal to the array type union will increase this nummber further by adding many more definitions - even though the issue was only about a specific method for *. It also seems a bit surprising to treat Diagonal in such a special way but not e.g. Adjoint, Symmetric and other matrix types in LinearAlgebra.

@tiemvanderdeure
Copy link

Has there been any progress on this?

Multiplying a Cholesky matrix with a Diagonal does not work with ReverseDiff, e.g. in the example shared here: TuringLang/Turing.jl#1870 (comment)

@devmotion
Copy link
Member

There's no multiplication of a Diagonal with a Cholesky matrix in the linked example? The model only multiplies a Diagonal with a LowerTriangular matrix. In any case, you could rewrite the multiplication with a Diagonal using broadcasting.

Generally, I still have the same feeling as in #231 (comment).

@tiemvanderdeure
Copy link

Sorry for being imprecise. The LowerTriangular comes from a Cholesky, though. It was recently implemented (which is great!) and I suspect I won't be the last one to try a similar operation.

The point you raised in #231 (comment) seems reasonable. I am pretty new to these ecosystem and have no feelings to this whatsoever. But I think it would be nice if models (like the one I linked) work as consistently as possible across AD backends. Or is this just not possible?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MethodError: *(::Diagonal, ::ReverseDiff.TrackedArray) is ambiguous.
3 participants