Add Symbolic Analysis of MTK System on Exiting Integration - #4720
Add Symbolic Analysis of MTK System on Exiting Integration#4720Shreyas-Ekanathan wants to merge 13 commits into
Conversation
AayushSabharwal
left a comment
AayushSabharwal
left a comment
There was a problem hiding this comment.
Apart from the piracy, it might be helpful to have a way to disable this as a solve kwarg or ODEProblem flag. Places that run solve in a hot loop (e.g. parameter optimization) typically want to avoid any sort of symbolic manipulation inside the loop. Alternatively, do we really need to identify the exact failing boolean expression? The user already has provided assertions in a way that makes sense for them so why does MTK need to do the extra effort?
| return term | ||
| end | ||
|
|
||
| function SciMLBase.diagnose_symbolic_instability(integrator::SciMLBase.DEIntegrator) |
There was a problem hiding this comment.
This is type piracy. MTK owns neither the function diagnose_symbolic_instability or the type DEIntegrator. Can this use something like get_root_indp in SciMLBase to obtain the system (if present) and dispatch on that?
AayushSabharwal
left a comment
AayushSabharwal
left a comment
There was a problem hiding this comment.
Just some performance/scaling comments.
| return isempty(diagnosis) ? "" : join(diagnosis, "\n") | ||
| end | ||
|
|
||
| function find_singular_subterms(eq, expr, sub_map, diagnosis) |
There was a problem hiding this comment.
If we need to substitute with the same sub_map over and over, it's a lot faster to build subber = SymbolicUtils.IRSubstituter{true}(get_irstructure(sys), sub_map) and call subber(expr) instead of substitute.
|
|
||
| #find singularity causes in equations | ||
| singularities = String[] | ||
| for eq in equations(sys) |
There was a problem hiding this comment.
We might want to run this on full_equations? equations are often trivial and we actually codegen full_equations. This would require using IRSubstituter to scale to the massive expressions Multibody has.
There was a problem hiding this comment.
If we do want to scale to full_equations, find_singular_subterms should also maintain an IdSet of expressions it has recursed into, so that it avoids recursing into the same subexpressions repeatedly. An example would be the same observed quantity substituted into multiple equations - it only needs to be reported and processed once.
There was a problem hiding this comment.
Should be addressed now, if you could take a look and lmk if anything else stands out that would be great!
Co-authored-by: Aayush Sabharwal <aayush.sabharwal@gmail.com>
Co-authored-by: Aayush Sabharwal <aayush.sabharwal@gmail.com>
AayushSabharwal
left a comment
AayushSabharwal
left a comment
There was a problem hiding this comment.
Just minor changes, looks good otherwise!
Co-authored-by: Aayush Sabharwal <aayush.sabharwal@gmail.com>
|
@AayushSabharwal can you rerun tests? the SciMLBase PR is merged now |
|
This then needs a compat bump on SciMLBase |
2 participants
Checklist
contributor guidelines, in particular the SciML Style Guide and
COLPRAC.
Additional context
Uses interface created in SciML/SciMLBase.jl#1434 and works with SciML/OrdinaryDiffEq.jl#3731 to analyze causes of instability