Skip to content

Conversation

@oscardssmith
Copy link
Member

This allows for custom initialization functions when using IDA.

@ChrisRackauckas
Copy link
Member

Add a test? I don't see how this works, and it doesn't disable the IDA initialization.

@oscardssmith
Copy link
Member Author

oscardssmith commented May 2, 2023

yeah, I wasn't sure what to do about the IDA initialization. The options I see are to add it as it's own initialization that can be used by anyone, stop using it entirely, or something else. Thoughts?

@oscardssmith oscardssmith reopened this May 2, 2023
@oscardssmith
Copy link
Member Author

This is now much closer to working. Tests still seem to be failing though. Something is slightly off.

@ChrisRackauckas
Copy link
Member

Rebase and add tests

@oscardssmith
Copy link
Member Author

This is almost ready. The remaining problem is that the OrdinaryDIffEq initialize_dae methods hardcode ODEIntegrator and the non-trivial ones rely on the caches that IDAIntegrators don't have. This is made more complicated because all of the initialization algorithms are defined in OrdinaryDiffEq rather than SciMLBaase so Sundials can't define it's own methods here. As such, I think this PR should probably be merged without us supporting the ODE initializations (since this PR as is allows for overriding IDA's initialization which is the main thing I care about here).

@staticfloat
Copy link
Contributor

The test failure looks real

@codecov
Copy link

codecov bot commented May 9, 2023

Codecov Report

Merging #401 (fddfc7f) into master (92891ee) will increase coverage by 1.09%.
The diff coverage is 100.00%.

❗ Current head fddfc7f differs from pull request most recent head 7fabbfd. Consider uploading reports for the commit 7fabbfd to get more accurate results

@@            Coverage Diff             @@
##           master     #401      +/-   ##
==========================================
+ Coverage   80.21%   81.31%   +1.09%     
==========================================
  Files          11       11              
  Lines        1466     1472       +6     
==========================================
+ Hits         1176     1197      +21     
+ Misses        290      275      -15     
Impacted Files Coverage Δ
src/common_interface/integrator_types.jl 71.42% <ø> (ø)
src/common_interface/integrator_utils.jl 84.76% <100.00%> (+0.52%) ⬆️
src/common_interface/solve.jl 82.13% <100.00%> (+0.08%) ⬆️

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@oscardssmith
Copy link
Member Author

test added.

function handle_callback_modifiers!(integrator::IDAIntegrator)
IDAReInit(integrator.mem, integrator.t, integrator.u, integrator.du)
DiffEqBase.initialize_dae!(integrator)
DiffEqBase.initialize_dae!(integrator, IDADefaultInit())
Copy link
Member

Choose a reason for hiding this comment

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

why the default here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought that after a callback the default initialization was the one we wanted. Is it not?

Copy link
Member

Choose a reason for hiding this comment

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

I don't recall if it was using the user-chosen one or the default.

Copy link
Member Author

Choose a reason for hiding this comment

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

OrdinaryDIffEq just calls integrator.reeval_fsal = true here. Previously, Sundials would just call DiffEqBase.initialize_dae!(integrator) which is now spelled DiffEqBase.initialize_dae!(integrator, IDADefaultInit()) (since now the initialization is customizable.

@oscardssmith
Copy link
Member Author

Can we merge this?

@ChrisRackauckas
Copy link
Member

It's missing the lower bound bump.

@oscardssmith
Copy link
Member Author

version bumped.

@ChrisRackauckas
Copy link
Member

It looks like your test case fails?

@ChrisRackauckas ChrisRackauckas merged commit bbebb76 into SciML:master May 17, 2023
@oscardssmith oscardssmith deleted the patch-1 branch May 17, 2023 14:53
Keno added a commit to Keno/Sundials.jl that referenced this pull request Jun 17, 2023
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
Keno added a commit to Keno/Sundials.jl that referenced this pull request Jun 17, 2023
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
ChrisRackauckas pushed a commit to Keno/Sundials.jl that referenced this pull request Jul 1, 2023
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
ChrisRackauckas pushed a commit that referenced this pull request Jul 1, 2023
Sundials.jl got an implementation of the OrdinaryDiffEq DAE initialization
interface in #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
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.

3 participants