PID for complex dtype fixes #391
Conversation
|
Pre-commit fail seems to be unrelated? |
There was a problem hiding this comment.
This LGTM! I'll leave this unmerged until we do the next Lineax release (itself blocked on the next JAX release).
I think the pre-commit fail is due to an upstream JAX change: they've changed how config should be imported, I think.
As for why Tsit5 wasn't already caught, I think the answer might just be that we have close-to-zero coverage for complex dtypes in Diffrax.
Randl
force-pushed
the
complex_pid
branch
2 times, most recently
from
1ffae28
to
d22fa54
Compare
| ( | ||
| diffrax.ConstantStepSize(), | ||
| diffrax.PIDController(rtol=1e-5, atol=1e-8), | ||
| diffrax.PIDController(rtol=1e-5, atol=1e-8, pcoeff=0.3, icoeff=0.3, dcoeff=0.0), |
There was a problem hiding this comment.
I'm a little concerned by this change. IIRC the _all_pairs thing was to try and reduce test times, which I think end up being very substantial for this many possible combinations. Do you know how much this changes things?
Btw, can you have this PR target dev instead please?
Other than that this LGTM!
There was a problem hiding this comment.
It takes 6 minute instead of 4 in CI, out of total 24 before/26 after. So it's like 10% runtime, but otherwise complex types are almost untested (since one non-default parameter is dtype)
|
Alright, LGTM! Thank you for the fixes as always. |
* Fix complex casting and types issues * Dependency version
Depends on patrick-kidger/lineax#91
Should fix #389
More generally, it appears that
caveatis not tested at all for solvers. From what I can see, the problem inTsit5is not caught by any test (though maybe the relevant test just doesn't run with complex dtypes?). Maybe having a large grid of tests for solver/parameter setups with simple DE can be helpful for these?