Rework VecJac Operator #272
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #272 +/- ##
==========================================
- Coverage 87.08% 86.62% -0.46%
==========================================
Files 21 21
Lines 1254 1286 +32
==========================================
+ Hits 1092 1114 +22
- Misses 162 172 +10
☔ View full report in Codecov by Sentry. |
|
This might have to be a breaking change. The previous API dropped things and a lot of code was written to work around it. But now the API is more strict which breaks code 😞 |
| ## Allowed Function Signatures | ||
|
|
||
| For Out of Place Functions: | ||
|
|
||
| ```julia | ||
| f(u, p, t) # t !== nothing | ||
| f(u, p) # p !== nothing | ||
| f(u) # Otherwise | ||
| ``` | ||
|
|
||
| For In Place Functions: | ||
|
|
||
| ```julia | ||
| f(du, u, p, t) # t !== nothing | ||
| f(du, u, p) # p !== nothing | ||
| f(du, u) # Otherwise |
There was a problem hiding this comment.
That also drops p and t after asking from them. JacVec basically only allows f(du, u) and f(u). So this one is a strict superset
There was a problem hiding this comment.
(I always pass in the general function and then get an error that f can't take p or t as inputs, which is kind of weird to do if the function takes them as inputs)
There was a problem hiding this comment.
Where is v?
Oh I see the confusion. I meant to say function signatures for the input and not the constructed operator
| # FIXME: FunctionOperator is terribly type unstable. It makes it `::Any` | ||
| # NOTE: We pass `p`, `t` to Function Operator but we always use the cached version from | ||
| # VecJacFunctionWrapper |
There was a problem hiding this comment.
Sounds like the right thing to do is fix FunctionOperator?
There was a problem hiding this comment.
the note part or the fixme part? The fixme part definitely needs fixing, but the note part is needed, because p and t need to be propagated via update_coefficients
pandtare no longer droppedVecJacmatches the size offuI will test with SciML/NonlinearSolve.jl#268 a bit before merging