Skip to content

Conversation

oxinabox
Copy link
Member

@oxinabox oxinabox commented Aug 9, 2021

There is probably a better place for this than the FAQ

@oxinabox oxinabox added the documentation Improvements or additions to documentation label Aug 9, 2021
@oxinabox oxinabox marked this pull request as draft August 9, 2021 17:48
@codecov-commenter
Copy link

codecov-commenter commented Aug 9, 2021

Codecov Report

Merging #428 (1a91eaf) into master (1014715) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #428   +/-   ##
=======================================
  Coverage   92.37%   92.37%           
=======================================
  Files          14       14           
  Lines         787      787           
=======================================
  Hits          727      727           
  Misses         60       60           

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 1014715...1a91eaf. Read the comment docs.

@oxinabox oxinabox marked this pull request as ready for review August 16, 2021 15:02
Copy link
Member

@willtebbutt willtebbutt left a comment

Choose a reason for hiding this comment

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

Broadly looks good. Just a thing about structural tangents.

Copy link
Member

@willtebbutt willtebbutt left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@mzgubic mzgubic left a comment

Choose a reason for hiding this comment

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

LGTM, just a few typos/suggestions

In many cases these all all tangents can be treated the same: tangent types overload a bunch of linear-operators, and the majority of functions used inside a pullback are linear operators.
If you find linear operators from Base/stdlibs that are not supported, consider openning an issue or PR on the [ChainRulesCorejl repo](https://github.com/JuliaDiff/ChainRulesCore.jl/).

### Natural tangents
Copy link
Member

Choose a reason for hiding this comment

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

Not objecting to adding this, but don't we already say what the differences between natural and structural tangents are somewhere in the docs?

Copy link
Member Author

Choose a reason for hiding this comment

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

In the design docs we probably do.
I am in-favour of having redundancy in the docs.
It makes them much more understandable, since noone reads them end-to-end.
Albeit, at the cost of making the docs harder to maintain.

They can be thought of as a wrapper of the value the computation returns.
In this sense they wrap either a natural or structural tangent.

!!! warning "You should to support AbstractThunk inputs even if you don't use thunks"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
!!! warning "You should to support AbstractThunk inputs even if you don't use thunks"
!!! warning "You should support `AbstractThunk` inputs even if you don't use thunks"

Copy link
Member Author

Choose a reason for hiding this comment

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

This headings of admonitions don't support markdown, as I recall

Comment on lines 188 to 196
Natural tangent types are the types you might feel the tangent should be.
These are a purely human notion, they are the types the user wants to use because they make the math easy.
Copy link
Member

Choose a reason for hiding this comment

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

I think this is a bit vague. Even if you don't provide an all-encompassing definition, you need to tell the reader roughly what you mean by "natural" at all, what distinction are you drawing? If they don't already know then all these disclaimers are pretty confusing.

I would do this by example. The simplest one would be ComplexF64, that's a struct, and by "natural" you mean a gradient which is another number, as opposed to "structural" you mean Tangent{Complex}(...). And after that, move on to more complicated ones.

Copy link
Member Author

@oxinabox oxinabox Aug 17, 2021

Choose a reason for hiding this comment

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

Are there not several examples in the next paragraph?

Copy link
Member

@mcabbott mcabbott Aug 17, 2021

Choose a reason for hiding this comment

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

There are, but you don't quite spell out what this word "natural" is being used to mean, before using it a bunch of times. I think that should go first: first define the terminology, then the clear-cut cases on both sides, then the tricky cases.

I mean, maybe this is done elsewhere, and the FAQ is revision... I don't have the global view clearly in my head.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok fair.

@oxinabox
Copy link
Member Author

In the interest of, subideal docs are better than no docs,
I am merging this now.
We can make follow ups to further improve later

@oxinabox oxinabox merged commit 2208660 into master Aug 17, 2021
@oxinabox oxinabox deleted the ox/whattypes branch August 17, 2021 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants