Clarify "writing good rules" documentation #468
Conversation
Codecov Report
@@ Coverage Diff @@
## master #468 +/- ##
==========================================
- Coverage 92.98% 86.43% -6.55%
==========================================
Files 14 13 -1
Lines 812 656 -156
==========================================
- Hits 755 567 -188
- Misses 57 89 +32
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Hmm, not sure about this one. The reason is that either order works since the projection is inserted into the thunk, see ProjectTo docstring. This is to allow thunk propagation.
While I do agree that it is preferable to add the @thunk outside project, I would argue that inserting a thunk complicates the point this section is trying to make.
However, perhaps adding a sentence at the end of this section to clarify the thunk/project order might be a good alternative?
|
I'm happy to change it to whatever IS the recommended way to do this. I just got quite confused by looking at the FAQ entries for |
Co-authored-by: Miha Zgubic <[email protected]>
|
Fair.
Whereas |
| While this is more verbose, it ensures that if an error is thrown during the `pullback` the [`gensym`](https://docs.julialang.org/en/v1/base/base/#Base.gensym) name of the local function will include the name you gave it. | ||
| This makes it a lot simpler to debug from the stacktrace. | ||
|
|
There was a problem hiding this comment.
This paragraph was in the @not_implemented section but seems to actually belong here!
There was a problem hiding this comment.
correct.
I guess text was inserted in wrong place
| While this is more verbose, it ensures that if an error is thrown during the `pullback` the [`gensym`](https://docs.julialang.org/en/v1/base/base/#Base.gensym) name of the local function will include the name you gave it. | ||
| This makes it a lot simpler to debug from the stacktrace. |
There was a problem hiding this comment.
Moved to "Code Style" at the top.
| ) | ||
| ``` | ||
|
|
||
| Do not use `@not_implemented` if the differential does not exist mathematically (use `NoTangent()` instead). | ||
|
|
||
| While this is more verbose, it ensures that if an error is thrown during the `pullback` the [`gensym`](https://docs.julialang.org/en/v1/base/base/#Base.gensym) name of the local function will include the name you gave it. | ||
| This makes it a lot simpler to debug from the stacktrace. | ||
| Note: [ChainRulesTestUtils.jl](https://github.com/JuliaDiff/ChainRulesTestUtils.jl) marks `@not_implemented` differentials as "test broken". |
There was a problem hiding this comment.
To document the new behaviour of JuliaDiff/ChainRulesTestUtils.jl#218
There was a problem hiding this comment.
It did that even before. for most cases.
The case it didn't handle was for things where the tangent was determined to be NoTangent.
But regardless this is a good change
st--
changed the title
oxinabox
changed the title
oxinabox
changed the title
|
thanks |
@thunkandProjectTo