Support for derivatives and Jacobians of complex-valued callables #17
Conversation
… done and should not be used directly.
Codecov Report
@@ Coverage Diff @@
## master #17 +/- ##
==========================================
+ Coverage 34.34% 40.33% +5.99%
==========================================
Files 1 4 +3
Lines 198 295 +97
==========================================
+ Hits 68 119 +51
- Misses 130 176 +46
Continue to review full report at Codecov.
|
1 similar comment
1 similar comment
src/finitediff.jl
Outdated
| if fdtype==Val{:forward} | ||
| return sqrt(eps(T)) | ||
| elseif fdtype==Val{:central} | ||
| return cbrt(eps(T)) | ||
| else | ||
| error("Unrecognized fdtype: must be Val{:forward} or Val{:central}.") | ||
| end | ||
| nothing |
There was a problem hiding this comment.
why nothing here? Shouldn't this error?
There was a problem hiding this comment.
The nothing is redundant there, it will never do anything, but it won't prevent the error. I probably just wrote it by reflex.
src/finitediff.jl
Outdated
| elseif eltype(x) <: Real | ||
| return eltype(x) | ||
| elseif eltype(x) <: Complex | ||
| return eltype(x).parameters[1] |
There was a problem hiding this comment.
is this type-inferrable?
There was a problem hiding this comment.
Probably better to:
julia> real(typeof(1.0+im))
Float64There was a problem hiding this comment.
Good point, changed it to real(eltype(x)).
| fx::Union{Void,AbstractArray{<:Real}}=nothing, epsilon::Union{Void,AbstractArray{<:Real}}=nothing, returntype=eltype(x), | ||
| df::Union{Void,AbstractArray{<:Real}}=nothing) | ||
|
|
||
| # TODO: test and rework this |
There was a problem hiding this comment.
test and rework what? Just this abstract dispatch?
There was a problem hiding this comment.
No, the implementation. It currently uses stuff like J[:,i], which will fail for matrix types that don't allow indexing. It's not a regression because we've never supported it, but it's not quite general enough for completely arbitrary matrices yet, and hasn't been tested on sufficiently exotic types.
|
I agree that it's a little messy, but it's probably better to keep iterating it in subsequent PRs instead of trying to stuff it into one "perfect PR" (that's how things never finish...). So I left a few comments and we can go from there. |
1 similar comment
This is far from perfect and requires major refactoring in the future (particularly when the constant propagation issues are resolved), but the new features generally work, the tests pass and there shouldn't be any regressions.