WIP: Docs on ProjectTo and embedded subspaces by oxinabox · Pull Request #412 · JuliaDiff/ChainRulesCore.jl

oxinabox · 2021-07-26T15:17:56Z

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.

codecov-commenter · 2021-07-26T15:21:23Z

Codecov Report

Merging #412 (67e95af) into master (d68a5b8) will not change coverage.
The diff coverage is n/a.

@@           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.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d68a5b8...67e95af. Read the comment docs.

sethaxen

I imagine there is more coming?

sethaxen · 2021-07-27T20:16:45Z

docs/src/embedded_subpaces.md

+# 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."_


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."_

sethaxen · 2021-07-27T20:16:52Z

docs/src/embedded_subpaces.md

+_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."_
+


Suggested change

sethaxen · 2021-07-27T20:17:01Z

docs/src/embedded_subpaces.md

+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 `*`


Suggested change

Consider the following possible `rrule` for `*`

Consider the following possible `rrule` for `*`:

sethaxen · 2021-07-27T20:17:43Z

docs/src/embedded_subpaces.md

+Consider the following possible `rrule` for `*`
+```julia
+function rrule(::typeof(*), a, b)
+    mul_pullback(ȳ) = (NoTangent(), ȳ*b', a'*ȳ)


Should you use the Δ convention here?

sethaxen · 2021-07-27T20:18:08Z

docs/src/embedded_subpaces.md

+julia> pb(1.0)
+(NoTangent(), 3.0, 2.0)
+```
+and for matrixes


Suggested change

and for matrixes

and for matrices:

sethaxen · 2021-07-27T20:25:37Z

docs/src/embedded_subpaces.md

+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:


Should this refer to a specific Julia version for simplicity?

sethaxen · 2021-07-27T20:26:04Z

docs/src/embedded_subpaces.md

+*(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).


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).

sethaxen · 2021-07-27T20:26:15Z

docs/src/embedded_subpaces.md

+
+---
+
+Consider the following possible `rrule` for `sum`


Suggested change

Consider the following possible `rrule` for `sum`

Consider the following possible `rrule` for `sum`:

sethaxen · 2021-07-27T20:27:09Z

docs/src/embedded_subpaces.md

+(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:


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:

sethaxen · 2021-07-27T20:27:20Z

docs/src/embedded_subpaces.md

+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.


Suggested change

You have escape the subspace that the diagonal type represents.

You have escaped the subspace that the diagonal type represents.

oxinabox · 2021-07-27T20:40:48Z

I imagine there is more coming?

Yep.
Was hoping on getting time for this today.
But instead did the last few things required to Zygote onto CRC 1.0, Nabla too 🎉

WIP: Docs on ProjectTo and embedded subspaces

67e95af

sethaxen self-requested a review July 26, 2021 16:07

sethaxen requested changes Jul 27, 2021

View reviewed changes

mcabbott added documentation Improvements or additions to documentation ProjectTo related to the projection functionality labels Nov 1, 2021

	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 `*`

	(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).

	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:

	You have escape the subspace that the diagonal type represents.
	You have escaped the subspace that the diagonal type represents.

Conversation

oxinabox commented Jul 26, 2021

Uh oh!

codecov-commenter commented Jul 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sethaxen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oxinabox commented Jul 27, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-commenter commented Jul 26, 2021 •

edited

Loading