Skip to content

Conversation

@mzgubic
Copy link
Member

@mzgubic mzgubic commented Jun 7, 2021

This builds off @nickrobinson251's https://github.com/JuliaDiff/ChainRulesCore.jl/pull/331/files and adds an example struct, as well as adds a note on functors. I've also moved the Code Style to the top, as the generic signature is the most common thing people want.

Closes #315

@mzgubic mzgubic requested a review from nickrobinson251 June 7, 2021 11:49
For the functor, use `bar::Bar`, i.e.

```julia
function ChainRulesCore.rrule(bar::Bar, x, y)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oxinabox just double checking this is right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is right.
Though I think that seperating constructors into a seperate section to Functors might be clearer.
and have the functor section say somehting like "In constrast to defined the rule for constructing an object, one can overload the rule for calling an instance of an object (if that object is a functor)"
or somehting like that.

Still even with them together this is a clear enhancement to the docs, so I am happy to have it merged and then to a follow up if you like

@codecov-commenter
Copy link

codecov-commenter commented Jun 7, 2021

Codecov Report

Merging #366 (c9f9168) into master (26b7e4a) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #366   +/-   ##
=======================================
  Coverage   87.23%   87.23%           
=======================================
  Files          13       13           
  Lines         478      478           
=======================================
  Hits          417      417           
  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 26b7e4a...c9f9168. Read the comment docs.


Do not use `@not_implemented` if the differential does not exist mathematically (use `NoTangent()` instead).

## Code Style
Copy link
Member

@oxinabox oxinabox Jun 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving code in the same PR as edits somehting else makes reviewing harder.
It's not terrible in this case since the PR is small, but still like 1/2 the changed lines are moving this)

a::Float64
end

(bar::Bar)(x, y) = return bar.a + x + y # functor
Copy link
Member

@oxinabox oxinabox Jun 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
(bar::Bar)(x, y) = return bar.a + x + y # functor
(bar::Bar)(x, y) = return bar.a + x + y # functor (i.e. callable object, overloading the call action)

@nickrobinson251
Copy link
Contributor

Thanks for taking over this! I'll leave Lyndon to review. I think Seth also had some comments on the previous PR :)

@nickrobinson251 nickrobinson251 removed their request for review June 7, 2021 12:04

```julia
function ChainRulesCore.rrule(bar::Bar, x, y)
...
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

including the code for the pullback i think is important.
Possibly also emphasize above that this is what that first arg is for.

Suggested change
...
# Notice the first return is not `NoTangent()`
Bar_pullback(z̄) = Tangent{Bar}(;a=z̄), z̄, z̄

To define an `rrule` for a constructor for a _type_ `Bar` we need to be careful to dispatch only on `Type{Bar}`.
For example, the `rrule` signature for a `Bar` constructor would be like:
```julia
function ChainRulesCore.rrule(::Type{Bar}, a)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Seth's point about <: (for non-concrete types) is worth making too
#331 (comment)

Copy link
Member

@oxinabox oxinabox Jun 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in general probably should always use <: inside Type.
Since it means your code doesn't have to change if it gains type-parameters, and thus is no longer concrete.
Plus saves thinking
(possibly even this should be added to BlueStyle)

@mzgubic mzgubic merged commit 6e2812e into master Jun 7, 2021
@mzgubic mzgubic deleted the mz/struct-docs branch June 7, 2021 12:44
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.

More docs/examples for structs/constructors

5 participants