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
base: master
Are you sure you want to change the base?
Feat: Handle Adjoints through Initialization #1168
Changes from 9 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
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.
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.
shouldn't default to ZygoteVJP. Should use the autojacvec of the ODE
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 9a8a845
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.