-
Notifications
You must be signed in to change notification settings - Fork 5
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also: you accidentally added guide.html, you should remove that.
Have you checked that it looks good rendered?
docs/src/pages/guide.md
Outdated
∇(v->f(v[1], v[2]))([x, y]) | ||
``` | ||
|
||
The methods considered so far have been completely generically typed. If one wishes to use methods whose argument types are restricted then one must surround the definition of the method in the `@unionise` macro. For example, if only a single definition is required: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to run this after method definition (e.g., if the method is in a package you don't control)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, unfortunately not. This is something I've been trying to figure out a decent solution for, but haven't come up with a great one yet. It seems possible that we should be able to write some code that generates a modified version of any particular package that a user wants to @unionise
, but you really do need to get access to the source somehow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can get access to the source code of a function with code_lowered
. I think this might be possible but it might also be A Bad Idea. I'd love to spend some time on that.
src/core.jl
Outdated
@@ -169,7 +169,7 @@ function ∇(f::Function, get_output::Bool=false) | |||
y = f(args_...) | |||
∇f = ∇(y) | |||
∇args = ([∇f[arg_] for arg_ in args_]...) | |||
return get_output ? (y, ∇args) : ∇args | |||
return get_output ? (y.val, ∇args) : ∇args |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean to include this in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, no I definitely did not.
docs/src/pages/guide.md
Outdated
@@ -0,0 +1,94 @@ | |||
# Getting Started | |||
|
|||
`Nabla.jl` has two interfaces, both of which we expose to the end user. We first provide a minimal working example with the low-level interface, and subsequently show how the high-level interface can be used to achieve similar results. More involved examples can be found [here](https://github.com/invenia/Nabla.jl/tree/master/examples). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The order below is reversed; you show the high-level interface and then the low-level interface.
docs/make.jl
Outdated
@@ -5,7 +5,9 @@ makedocs( | |||
format=:html, | |||
pages=[ | |||
"Home" => "index.md", | |||
"API" => "pages/api.md" | |||
"API" => "pages/api.md", | |||
"Getting Started" => "pages/guide.md", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you might as well put the contents of guide.md in index.md, as there is nothing there right now.
docs/src/pages/guide.md
Outdated
``` | ||
|
||
## Gotchas and Best Practice | ||
- `Nabla.jl` does not currently have complete coverage of the entire language due to finite resources and competing priorities. Particularly notable omissions are the subtypes of `Factorization` objects and all in-place functions. These are both issues which will be resolved in the future. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/language/standard library/
Codecov Report
@@ Coverage Diff @@
## master #62 +/- ##
=======================================
Coverage 99.78% 99.78%
=======================================
Files 18 18
Lines 471 471
=======================================
Hits 470 470
Misses 1 1
Continue to review full report at Codecov.
|
|
||
We may provide an optional argument to also return the value `f(x, y)`: | ||
```@example toy | ||
(z, (∇x, ∇y)) = ∇(f, true)(x, y) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The output here
(Branch{Float64} -2.6250968532458, f=At_mul_B, ([0.713575, -1.19715], [-0.783863, -3.29845]))
looks like a 3-tuple, due to the comma before f=...
.
(z, (∇x, ∇y)) = ∇(f, true)(x, y) | ||
``` | ||
|
||
If the gradient w.r.t. a single argument is all that is required, or a subset of the arguments for an N-ary function, we recommend closing over the arguments which respect to which you do not wish to take gradients. For example, to take the gradient w.r.t. just `x`, one could do the following: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though the examples illustrate so, it might be good to explicitly remark that taking the gradient of a single-output functions yields a 1-tuple containing the gradient.
docs/src/index.md
Outdated
x_, y_ = Leaf.(Tape(), (x, y)) | ||
``` | ||
Note that it is critical that `x_` and `y_` are constructed using the same `Tape` instance. Currently, `Nabla.jl` will fail silently if this is not the case. | ||
We then simply pass `x_` and `y_` to `f` instead of `x` and `y`, and call `∇` on the result: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The clause and call
∇ on the result:
might be redundant, given line 93.
docs/src/index.md
Outdated
## Gotchas and Best Practice | ||
- `Nabla.jl` does not currently have complete coverage of the entire standard library due to finite resources and competing priorities. Particularly notable omissions are the subtypes of `Factorization` objects and all in-place functions. These are both issues which will be resolved in the future. | ||
- The usual RMAD gotcha applies: due to the need to record each of the operations performed in the execution of a function for use in efficient gradient computation, the memory requirement of a programme scales approximately linearly in the length of the programme. Although, due to our use of a dynamically constructed computation graph, we support all forms of control flow, long `for` / `while` loops should be performed with care, so as to avoid running out of memory. | ||
- In a similar vein, develop a (strong) preference higher-order functions and linear algebra over for-loops; `Nabla.jl` has optimisations targetting Julia's higher-order functions (`broadcast`, `mapreduce` and friends), and consequently loop-fusion / "dot-syntax", and linear algebra operations which should be made use of where possible. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is a "for" missing here?
... develop a (strong) preference ??? higher-order ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! :) I left behind a couple of comments.
Thanks for the useful reviews guys. I'll wait for CI stuff to run on the changes I've made in response to @wesselb's comments and then merge. |
Some actual documentation. This by no means represents the completion of #16, but I feel that it's a reasonable intermediate step in the mean time.
@iamed2 if you could let me know if it's interpretable that would be great. Hopefully this will be a 5 minute job.
@wesselb I think you took a look at this before, so if you could just give a quick look over again to make sure there's nothing obviously wrong I would really appreciate it.