-
-
Notifications
You must be signed in to change notification settings - Fork 233
Fix Jacobian sparsity pattern with observed derivatives #3949
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
Conversation
4d13c7a to
0dac698
Compare
0dac698 to
16c9c0d
Compare
|
It looks to me like the relevant tests are passing. Do you have any input on this @ChrisRackauckas? |
| sparsity = torn_system_jacobian_sparsity(sys) | ||
| sparsity === nothing || return sparsity | ||
| # disable to fix https://github.com/SciML/ModelingToolkit.jl/issues/3871 | ||
| #sparsity = torn_system_jacobian_sparsity(sys) | ||
| #sparsity === nothing || return sparsity |
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.
@AayushSabharwal is there a reason the torn sparsity would have false zeros here? That seems like it would give issues in other places.
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'm not 100% sure, we've just used torn_system_jacobian_sparsity since the beginning of time iirc. Maybe this doesn't handle the W sparsity correctly?
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.
Well I would think that if this sparsity pattern is wrong, then the incidence matrix is wrong. Can you make a note to follow up on this? For now I want to merge so that these are right, but I'd like to make sure something isn't hiding 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.
Will do
|
Thanks. I noticed it looks like |
I want to fix #3871.
I figured
torn_system_jacobian_sparsitycomputes an incorrect sparsity pattern for this example.My first idea is to disable it and always compute
jacobian_sparsitywith the "fallback method"Symbolics.jacobian_sparsity(...).If this does not work in all cases, the alternative would be to patch
torn_system_jacobian_sparsitydirectly.