WIP: Docs on ProjectTo and embedded subspaces #412
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #412 +/- ##
=======================================
Coverage 92.12% 92.12%
=======================================
Files 14 14
Lines 775 775
=======================================
Hits 714 714
Misses 61 61 Continue to review full report at Codecov.
|
sethaxen
requested changes
Jul 27, 2021
| # Types that represent embedded subspaces | ||
| _Taking Types Representing Embedded Subspaces Seriously™_ | ||
|
|
||
| To paraphrase Stefan Karpinski: _"This does mean treating sparse matrixes not just as a representation of dense matrixes, but the alternative is just too horrible."_ |
There was a problem hiding this comment.
This is phrased strangely and hard to follow.
Suggested change
| To paraphrase Stefan Karpinski: _"This does mean treating sparse matrixes not just as a representation of dense matrixes, but the alternative is just too horrible."_ | |
| To paraphrase Stefan Karpinski: _"This does mean treating sparse matrixes not just as a representation of dense matrixes, but the alternative is just too horrible."_ |
| _Taking Types Representing Embedded Subspaces Seriously™_ | ||
|
|
||
| To paraphrase Stefan Karpinski: _"This does mean treating sparse matrixes not just as a representation of dense matrixes, but the alternative is just too horrible."_ | ||
|
|
| To paraphrase Stefan Karpinski: _"This does mean treating sparse matrixes not just as a representation of dense matrixes, but the alternative is just too horrible."_ | ||
|
|
||
|
|
||
| Consider the following possible `rrule` for `*` |
There was a problem hiding this comment.
Suggested change
| Consider the following possible `rrule` for `*` | |
| Consider the following possible `rrule` for `*`: |
| Consider the following possible `rrule` for `*` | ||
| ```julia | ||
| function rrule(::typeof(*), a, b) | ||
| mul_pullback(ȳ) = (NoTangent(), ȳ*b', a'*ȳ) |
There was a problem hiding this comment.
Should you use the Δ convention here?
| julia> pb(1.0) | ||
| (NoTangent(), 3.0, 2.0) | ||
| ``` | ||
| and for matrixes |
There was a problem hiding this comment.
Suggested change
| and for matrixes | |
| and for matrices: |
| This seems even worse though: no only has it escaped the `Diagonal` type, it has even escaped the subspace it represents. | ||
|
|
||
| Further, it is inconsistent with what we would get had we AD'd the function for `*(::Diagonal, ::Matrix)` directly. | ||
| [That primal function](https://github.com/JuliaLang/julia/blob/f7f46af8ff39a1b4c7000651c680058e9c0639f5/stdlib/LinearAlgebra/src/diagonal.jl#L224-L245) boils down to: |
There was a problem hiding this comment.
Should this refer to a specific Julia version for simplicity?
| *(a::Diagonal, b::AbstractMatrix) = a.diag .* b | ||
| ``` | ||
| By reading that primal method, we know that that ADing that method would have zeros on the off-diagonals, because they are never even accessed | ||
| (a similar argument can be made for the complex part of a real number). |
There was a problem hiding this comment.
Suggested change
| (a similar argument can be made for the complex part of a real number). | |
| (a similar argument can be made for the imaginary part of a real number). |
|
|
||
| --- | ||
|
|
||
| Consider the following possible `rrule` for `sum` |
There was a problem hiding this comment.
Suggested change
| Consider the following possible `rrule` for `sum` | |
| Consider the following possible `rrule` for `sum`: |
| (NoTangent(), [1.0 1.0; 1.0 1.0]) | ||
| ``` | ||
| That's not right -- not if us saying this was `Diagonal` meant anything. | ||
| If you try and use that dense matrix, to do gradient descent on your `Diagonal` input, you will get a non-diagonal result: |
There was a problem hiding this comment.
Suggested change
| If you try and use that dense matrix, to do gradient descent on your `Diagonal` input, you will get a non-diagonal result: | |
| If you try and use that dense matrix to do gradient descent on your `Diagonal` input, you will get a non-diagonal result: |
| That's not right -- not if us saying this was `Diagonal` meant anything. | ||
| If you try and use that dense matrix, to do gradient descent on your `Diagonal` input, you will get a non-diagonal result: | ||
| `[2.0 1.0; 1.0 2.0]`. | ||
| You have escape the subspace that the diagonal type represents. |
There was a problem hiding this comment.
Suggested change
| You have escape the subspace that the diagonal type represents. | |
| You have escaped the subspace that the diagonal type represents. |
Yep. |
mcabbott
added
documentation
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is kinda the follow up to https://github.com/JuliaDiff/ChainRulesCore.jl/pull/347/files
but including our actual resolution of it.
It still needs to move some content over from that PR.