Revive pss_graph_modia!
#2192
Merged
Revive pss_graph_modia!
#2192
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Can we add this as an option in ddg? It seems like a different priority for nonlinear equations. |
When I originally wrote this code, I didn't really understand how it was supposed to work, simply copying the algorithm from modia. As a result, the algorithm was somewhat incomplete, and we ended up switching to dummy_derivative_graph! instead. However, we are now seeing issues where the state selection from `dummy_derivative_graph!` (ddg) is resulting in state selections that are numerically sub-optimal because ddg does not respect solvability. With the benefit of hindsight, fix pss_graph_modia! to be actually correct. The keen eyed reader will notice that the changes to pss_graph_modia! are essentially exactly what ddg does right now, except that I don't currently implement the linear case. I think it would make sense to further unify this and fully switch over MTK to the new `pss_graph_modia!`, but past experience has shown the MTK/Downstream tests to be quite sensitive to the exact state selection, so this does not attempt to make that change yet.
|
Yes, we should unify these. However, I'd like to do that in a separate PR. This function still needs support for the linear exact jacobian that ddg has, so I'd just like to do that all at once. |
YingboMa
approved these changes
Jun 13, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
When I originally wrote this code, I didn't really understand how it was supposed to work, simply copying the algorithm from modia. As a result, the algorithm was somewhat incomplete, and we ended up switching to dummy_derivative_graph! instead. However, we are now seeing issues where the state selection from
dummy_derivative_graph!(ddg) is resulting in state selections that are numerically sub-optimal because ddg does not respect solvability. With the benefit of hindsight, fix pss_graph_modia! to be actually correct.The keen eyed reader will notice that the changes to pss_graph_modia! are essentially exactly what ddg does right now, except that I don't currently implement the linear case. I think it would make sense to further unify this and fully switch over MTK to the new
pss_graph_modia!, but past experience has shown the MTK/Downstream tests to be quite sensitive to the exact state selection, so this does not attempt to make that change yet.