- Sponsor
-
Notifications
You must be signed in to change notification settings - Fork 225
fix linearization t0 and bump di #3733
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?
Conversation
I'm not sure I understand the implications of |
The part that's a little weird here is that this api is intended to work for
|
Okay it's odd to build an ODEProblem with |
What if we build with |
That would be fine if most of the ODEProblem is discarded. |
I think the only reason we're building an ODE in the first place is to get OrdinaryDiffEq jacobian handling. Now that DI exists, we could probably just use it directly. That said, I think this is a good fix in the short term. |
so MWE of the issue for @AayushSabharwal:
gives
Specifically, the problem comes from |
@@ -716,7 +717,7 @@ lsys_sym, _ = ModelingToolkit.linearize_symbolic(cl, [f.u], [p.x]) | |||
@assert substitute(lsys_sym.A, ModelingToolkit.defaults(cl)) == lsys.A | |||
``` | |||
""" | |||
function linearize(sys, lin_fun::LinearizationFunction; t = 0.0, | |||
function linearize(sys, lin_fun::LinearizationFunction; t = nothing, |
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.
Why was this kwarg changed? That's the root cause for the failures
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.
#3733 (comment). The previous problem was that we were preparing the ODE with t=nothing and running with t=0 which meant our jacobian was getting an unexpected t
type.
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.
Basically the problem is that linearize
has this weird spec where we aren't requiring a t
for autonomous systems, but otherwise requires a t
to be given.
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.
So defaulting to t = 0
for autonomous systems shouldn't be a problem?
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.
MTK#master right now accepts t
as a kwarg to linearization_function
, and forwards it from linearize
. So that should just address 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.
I still don't get why we don't want to default to t=0.0
for non-autonomous systems 😅 The higher level API for this is linearize
and that defaults to t=0.0
. And the t
provided here is essentially only for typing the DI jacobian prep. The lin_fun::LinearizationFunction
returned from linearization_function
is called as (u, p, t)
which requires passing time, and if the user calls linearize(lin_fun, sys)
they can also pass a new t
there. This behavior is essentially no different from what we had before. Making t = nothing
, defaulting to zero for autonomous and erroring non-autonomous is an orthogonal change and can be done separately.
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.
This PR also removes that as a default unless I messed something up. But also, why should it be Float64
only? If the user passes a Float32 time, do we expect an error here (due to incorrect prep)?
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.
Defaulting to t=0
is perfectly reasonable, the point in which to linearize is also by default the same as the initial condition. Changing this is also breaking since this has been the default before.
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 t=0
guaranteed to be an initial condition?
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, neither are default values in standard library component, it will just be used if no explicit override is provided.
The time at which linearization was performed is part of the return structure, so in the very odd case where someone satisfies all of
- Has a non-autonomous system
- The linearizaiton actually depends on
t
(most of the time it does not even for non-autonomous systems) - They forgot to pass
t
and were confused about the result
they will still get a result structure that contains t=0
making this rather explicit.
For reference, my vscode workspace contains 506 results over 123 files for the search term linearize
, the breakage would be enormous if we suddenly started requiring the user passing in t
(I have never needed this before)
No description provided.