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.
Feat: Handle Adjoints through Initialization #1168
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
Feat: Handle Adjoints through Initialization #1168
Changes from 55 commits
d6290bf
d3199c0
a4fa7c5
5a7dd26
94ec324
0c5564e
aca9cd4
9bec784
72cbb35
2d85e19
9a8a845
95ebbf3
957d7fe
a00574f
4562f0c
6c21324
a675a7f
7941a3c
9557e8c
8feae0e
d3b1669
e01eb77
0ad6c62
6df7987
c4c7807
b85b16e
8fc4136
1f1cce5
0d1abcc
b164b18
915d949
d2fd79a
a0cd94a
885794d
13a1ffb
7826866
6ceaa1a
dfebd0b
396f63e
de7e7da
f5fb559
019a051
91ee019
4b74718
94d5e2b
764d3ff
84b2602
8a4aa79
d69ccb1
2cc3673
22f056a
1f95b25
0d78fa8
6620e8a
5f5633b
984c2ce
82cd5fe
987e8be
056fffa
a122340
e105838
aaddc02
60be1c7
9edbe02
308ae5c
a333588
a2cf0e6
755c9df
75ad141
e07dd53
ee804b2
7abf42c
a7d4e5a
8e660fe
de63cf9
b88f468
7dd1cc7
cdaa2c7
d3608c4
4cf7bd5
2ae712b
229e691
6e1109e
d32b3f2
6e549e7
9789034
35937c0
f934635
a83cd29
2dfbc6e
9aecbfd
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same down here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also needs to check that initializealg is not set, is the default, or is using OverrideInit. Should test this is not triggered with say manual BrownBasicInit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. My understanding was that OverrideInit was what we strictly needed. We can check for BrownBasicInit/ defaults here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There doesn't seem to be a method which can take a
BrownFullBasicInit()
. I get aMethodError
:Only
CheckInit
,NoInit
, andOverrideInit
have dispatches.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having chatted with @AayushSabharwal on this, it seems like BrownBasic and ShampineCollocation do not yet have a path through
get_initial_values
and that would need to be fixed in OrdianryDiffEq. Further, as SciMLSensitivity does not depend on OrdinaryDiffEq, it cannot check for whether there is a default initialisation.since those are defined there. Depending on it also seems like a pretty big hammer for a dep.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would be the best course of action here? Seems like supporting BrownBasicInit is a dispatch that will automatically be utilised when it is moved into SciMLBase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I mean BrownBasicInit should not be taking this path. But that's a problem because then they will be disabled in the next stage below, and that needs to be accounted for. This dispatch is already built and setup for BrownBasicInit and there are tests on that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These don't make sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, these should probably inherit from
kwargs
or be set up to some default. Note that we must specify a tol for this dispatch.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in 984c2ce
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should only noinit if the previous case was ran. Won't this right now break the brownbasic tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there was no intialization data, it won't have ran the initialization problem at all.
If I can genetically ignore handling
initializealg
and pass it directly toget_initial_values
, that would be good. Then I can also passNoInit
here genetically.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No that is not correct. If there was no initialization data then it will use the built in initialization, defaulting to BrownBasicInit. It's impossible for a DAE solver to generally work without running initialization of some form, the MTK one is just a new specialized one but there has always been a numerical one in the solver. And if it hits that case, this code will now disable that.
https://github.com/SciML/SciMLSensitivity.jl/blob/master/test/adjoint.jl#L952-L978 this code will hit that. I think it's not failing because it's not so pronounced here. You might want to change that test to https://github.com/SciML/SciMLSensitivity.jl/blob/master/test/adjoint.jl#L975C5-L975C69
prob_singular_mm = ODEProblem(f, [1.0, 0.0, 1.0], (0.0, 100), p)
and it would pass before and fail now.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right of course for the DAEs, but since
BrownBasicInit
is defined in OrdinaryDiffEq, and this package does not depend on it, I need a way for us to be able to dispatch to it. So if I understand the comment from earlier, we need a check for the default initialization, and add a branch that solves for thatprob
, and collect all the outputs.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both
BrownBasicInit
andOrdinaryDiffEqCore.DefaultInit
require us to depend on a whole package for the default dispatch. Can it be exposed as a dispatch ofget_initial_values
instead?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh 😅 . That case was too simple, MTK turns it into an ODE. Let's make it a DAE.
That should make it so that it eliminates the
w
term, but doesn't eliminate thew2
term. The DAE check is on the w2 term, the observed handling check is on the w term.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That will need to change the integrator to
Rodas5P
, Tsit5 will not be compatible with this form.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For SDEs, we will just need to make it compatible with BrownBasicInit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I was so confused why it worked out, I see the
InitialFailure
now.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay good. Yeah because MTK is too smart and makes lots of simple examples not DAEs 😅. But now you got the DAE, and if not running the built in init then you get the error I was expecting. The fix is that it needs to run brownbasic before solving for the same reason reverse needs to. Good we worked out a test for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the new reverse ode is built it needs to drop the initial eqs but still keep the dae constraints. It can brownbasic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to drop the initial eqs after its solved? The assumption was since we run with
NoInit
, no initialization is run post the first call toget_initial_values
and we accumulate those gradients independently of the adaptive solve.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the reverse pass needs to run with some form of initialization or the starting algebraic conditions may not be satisfied. Don't run this one with NoInit(), that would be prone to hiding issue. For this one, at most CheckInit(), but I'm saying that BrownBasicInit() is likely the one justified here since the 0 initial condition is only true on the differential variables, while the algebraic variable initial conditions will be unknown, but the Newton solve will have zero derivative because all of the inputs are just Newton guesses, so BrownBasic will work out for the reverse. We should probably hardcode that since it's always the solution there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, that will require us to add an OrdinaryDiffEqCore dep in this package. I will add that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the 0 derivative also applicable to parameters? Or only the unknowns?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its applicable to all Newton guess values. There is no parameter init going on to reverse so it's only for algebraic conditions so it's only Newton guesses.