Correct OrdinaryDiffEq DAE init integration #407
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@oscardssmith Can you put together some reduced tests for all this? |
Keno
added a commit
to Keno/OrdinaryDiffEq.jl
that referenced
this pull request
Jun 17, 2023
Found by inspection - I think this acidentally had the same bug as SciML/Sundials.jl#407 if save_idxs is not `nothing`.
|
test added. |
|
Test fails |
|
yeah a different test is failing. I'm pretty sure some of the changes here are invalid, but I'm investigating. |
Sundials.jl got an implementation of the OrdinaryDiffEq DAE initialization interface in SciML#401. Unfortunately, it turns out that this doesn't quite work, because the u/du in Sundials' integrator struct are not the actual memory that IDA uses (it has an internal copy). In [1], I propose re-using `u_modified` to inform us that the shadow copy was modified during initialization. This is the Sundials side of that interface. [1] SciML/OrdinaryDiffEq.jl#1969
As in OrdinaryDiffEq's version of this function, we should save_start after DAE initialization has had a chance to change the initial guess.
ChrisRackauckas
force-pushed
the
kf/correctdaeinit
branch
from
12d01d4 to
eecd601
Compare
Codecov Report
@@ Coverage Diff @@
## master #407 +/- ##
==========================================
+ Coverage 81.44% 81.72% +0.28%
==========================================
Files 11 11
Lines 1428 1428
==========================================
+ Hits 1163 1167 +4
+ Misses 265 261 -4
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
SciML/OrdinaryDiffEq.jl#1969 is required for this to actually do something with the OrdinaryDiffEq inits. @oscardssmith can you make sure to add a test case that exercises that path? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This corrects two issues with the DAE init integration added in #401:
It makes sure that shadow copy of the state in the integrator struct is reflected back into IDA (with Set u_modified in DAE initialization if the u was changed OrdinaryDiffEq.jl#1969). This needs to handle two distinct cases:
a. Where a DiffEq DAE initialization algorithm is used directly (handled by checking u modified after the call to DAE init)
b. Where a DiffEq DAE init is chained into the IDA default init (handled by the check at the top of IDADefaultInit.
It also needs to make sure to reflect the values that IDADefaultInit came up with back into integrator.u to make sure that it can be read from there for anybody who might wish to inspect it.
The order of save_start and initialize_dae! was reversed, causing the first entry of the solution to always be
prob.u0rather than the result of DAE initialization.