Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PID for complex dtype fixes #391

Merged
merged 2 commits into from
Apr 22, 2024
Merged

Conversation

Randl
Copy link
Contributor

@Randl Randl commented Mar 22, 2024

Depends on patrick-kidger/lineax#91
Should fix #389

More generally, it appears that caveat is not tested at all for solvers. From what I can see, the problem in Tsit5 is 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?

@Randl
Copy link
Contributor Author

Randl commented Mar 22, 2024

Pre-commit fail seems to be unrelated?

Copy link
Owner

@patrick-kidger patrick-kidger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

diffrax/_solver/tsit5.py Outdated Show resolved Hide resolved
(
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),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@Randl Randl changed the base branch from main to dev April 21, 2024 20:16
@patrick-kidger patrick-kidger merged commit 41c58a6 into patrick-kidger:dev Apr 22, 2024
2 checks passed
@patrick-kidger
Copy link
Owner

Alright, LGTM! Thank you for the fixes as always.

patrick-kidger pushed a commit that referenced this pull request May 19, 2024
* Fix complex casting and types issues

* Dependency version
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Complex input in diffeqsolve with PIDController
2 participants