Battle testing round 1 #78
Conversation
…ing. Added some new scalar sensitivites in scalar.jl and changed the BenchmarkTools.jl tests requirement to be 0.0.8 and upwards. This dramatically accelerates testing.
…ual number, and hence the sensitivity w.r.t. an input is in fact zero, an error occurs. Instead, we now assume that a non-Dual number output -> sensitivity = 0.
…sensitivites now return a Diagonal matrix in all scenarios, which implies linear scaling in the length of the Diagonal N, rather than a dense matrix, which implies O(N^2) asymptotic memory and time complexity. Furthermore, the in-place optimisations have been improved slightly to handle Diagonal sensitivity caches better.
…es are returned from the optimisations. Also reformmated some code and changed the test requirements to place fewer restrictions on the version of BenchmarkTools that can be used when testing.
…tion at which stuff fails, such that the error can be easily reproduced.
Codecov Report
@@ Coverage Diff @@
## master #78 +/- ##
==========================================
- Coverage 99.81% 98.03% -1.78%
==========================================
Files 19 18 -1
Lines 535 509 -26
==========================================
- Hits 534 499 -35
- Misses 1 10 +9
Continue to review full report at Codecov.
|
…tional.jl and changed time at which coverage report is submitted.
| """ | ||
| function unionise_struct(code::Expr) | ||
| tmp = code.args[2] | ||
| name = tmp isa Expr && tmp.head == Symbol("<:") ? code.args[2].args[1] : code.args[2] |
There was a problem hiding this comment.
Perhaps store tmp isa Expr && tmp.head == Symbol("<:") in a variable since you're reusing it two lines below.
| `code` should be an `Expr` containing the definition of a type. The type will only be | ||
| changed if it is parametric. That is, if you wish to be able to differentiate through a | ||
| user-defined type, it must contain only `Any`s and parametric types. | ||
| """ |
There was a problem hiding this comment.
Should unionise_struct also Nablafy the fields of the struct?
There was a problem hiding this comment.
I don't think so. In general that would mean you would have to store pointers to the values in the fields in structs; it wouldn't make any difference for mutable structs though. i.e.
struct Foo
x::Union{Float64, Node{<:Float64}}
endstores a pointer to x on the heap, whereas
struct Foo
x::Float64
endjust stores x.
| Expr(Symbol("::"), arg.args[1:end-1]..., unionise_type(arg.args[end])) : | ||
| arg.head == Symbol("...") ? | ||
| Expr(Symbol("..."), unionise_arg(arg.args[1])) : | ||
| throw(error("Unrecognised argument in arg ($arg).")) |
There was a problem hiding this comment.
Unrecognised symbol is perhaps more descriptive.
src/finite_differencing.jl
Outdated
|
|
||
| # Both of these functions are technically defined on the entire real line, but the negative | ||
| # half is troublesome due to the large number of points at which it isn't defined. As such | ||
| # we restrict unit testing to the left-half line. |
src/sensitivities/scalar.jl
Outdated
| # TODO: Make a PR for DiffRules. | ||
| @define_diffrule Base.:\(x, y) = :(-$y / $x^2), :(1 / $x) | ||
| @define_diffrule Base.:transpose(x) = :(1) | ||
| @define_diffrule Base.:abs(x) = :($x > 0 ? one($x) : -one($x)) |
There was a problem hiding this comment.
Would it make sense to let abs'(0) = 0?
There was a problem hiding this comment.
So my (somewhat silly) argument is to pretend that abs is actually defined as
abs(x) = x > -eps ? x : -xwhere eps is some constant smaller than whatever value your floating point numbers are able to represent (not machine epsilon), such that we never actually hit the discontinuity when we evaluate abs, and consequently never have to handle it when we are evaluating the gradient.
In short, I really don't think that defining the gradient of abs to be zero at zero makes sense.
There was a problem hiding this comment.
That makes sense, though 0 can be represented exactly, which would hit the discontinuity.
There was a problem hiding this comment.
Perhaps it is most sensible to let abs'(0) = NaN.
src/sensitivities/indexing.jl
Outdated
| @@ -5,7 +5,11 @@ for i = 1:7 | |||
| @eval @explicit_intercepts getindex $T [[true]; fill(false, $i - 1)] | |||
| end | |||
|
|
|||
| ∇(Ā, ::typeof(getindex), ::Type{Arg{1}}, p, y, ȳ, A, inds...) = setindex!(Ā, ȳ, inds...) | |||
| # ∇(Ā, ::typeof(getindex), ::Type{Arg{1}}, p, y, ȳ, A, inds...) = setindex!(Ā, ȳ, inds...) | |||
There was a problem hiding this comment.
Perhaps just remove the line as opposed to commenting it out.
Making a load of updates to as I use it to implement a separate package. This has thus far proved to be a useful exercise as it is has already highlighted a number of issues and missing features that I am implementing as I proceed.
There will likely be a few new linear algebra optimisations as well, which is really the name of the game at the moment.