Skip to content
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

Re-enable conservation laws for NonlinearSystems #1206

Merged
merged 32 commits into from
Apr 8, 2025
Merged

Conversation

TorkelE
Copy link
Member

@TorkelE TorkelE commented Mar 30, 2025

Needs some checking, but this should re-enable the use of conservation law elimination for NonlinearSystems using the approach in SciML/ModelingToolkit.jl#3458 (comment). Basically requires:

  • Re-adding the conservation law to the equations.
  • Removing the conservation law from the observables list.
  • Creating initialization_eqs with the conservation laws (but with species replaced with their initial value parameter.
  • Adding the eliminated species back as species of the system.

EDIT, this is not actually the case, my mistake: The updated approach will not enable us to deal with singular Jacobians, which sucks. In the case of Bifurcation diagrams and homotopy continuation this is handled separately though so no problem there (but a problem for normal non-linear system solving).

@TorkelE TorkelE changed the title Re-enable conservation laws for NonlinearSystems [WIP] Re-enable conservation laws for NonlinearSystems Mar 30, 2025
@ChrisRackauckas
Copy link
Member

The updated approach will not enable us to deal with singular Jacobians, which sucks.

But it gets rid of singular Jacobians?

@TorkelE
Copy link
Member Author

TorkelE commented Mar 30, 2025

No, you are right, I just didn't look through things properly yet 😅

@ChrisRackauckas
Copy link
Member

Have you tried using the standard MTK HC interface? I would hope the Catalyst one isn't needed anymore, and it should make things like hill functions now handled through the rationalization passes.

@TorkelE
Copy link
Member Author

TorkelE commented Apr 1, 2025

Doesn't that still eliminate all but one of the solutions though? Last time I checked it only returned a single solution, which mostly defeated the purpose of using it (it might have been updated since, but cannot find anything about it). Long-term using the MTK interface is still the plan though.

@isaacsas
Copy link
Member

isaacsas commented Apr 1, 2025

@ChrisRackauckas can you point us to the official documentation on the MTK interface? Searching the last MTK release's docs for homotopy doesn't seem to give any results?

@isaacsas
Copy link
Member

isaacsas commented Apr 1, 2025

In either case, we can keep the Catalyst interface for now, and when the MTK interface is documented update the tutorial to show its usage too (or recommend it as the preferred approach if it encompasses all the Catalyst functionality). Adding that wouldn't be breaking, so can wait to be handled till post V15 being released.

@ChrisRackauckas
Copy link
Member

It's now just a solver alg in NonlinearSolve.jl

https://docs.sciml.ai/NonlinearSolve/stable/api/homotopycontinuation/

We do need to figure out how to document that better. The AllRoots option controls the return @TorkelE .

@ChrisRackauckas
Copy link
Member

Though for the depolynomialization you'll want to target it through https://github.com/SciML/ModelingToolkit.jl/blob/master/src/systems/nonlinear/homotopy_continuation.jl#L536.

I'll chalk this up to the fact that MTK kind of needs a complete doc structure overhaul. Maybe we can dedicate one of the upcoming symbolic numerics thrusday meetings to that and just get it done.

@TorkelE did you de-register the Hill functions? Since that would be required for the rationalization.

@isaacsas
Copy link
Member

isaacsas commented Apr 1, 2025

I'll chalk this up to the fact that MTK kind of needs a complete doc structure overhaul. Maybe we can dedicate one of the upcoming symbolic numerics thrusday meetings to that and just get it done.

That would be great, especially in clarifying what is public API for Catalyst (or other libraries) to use, and what is internal MTK API.

@TorkelE
Copy link
Member Author

TorkelE commented Apr 1, 2025

@TorkelE did you de-register the Hill functions? Since that would be required for the rationalization.

still registered, but we can probably change. Not sure what the wider implications for various cases would be though.

@TorkelE
Copy link
Member Author

TorkelE commented Apr 1, 2025

I'll chalk this up to the fact that MTK kind of needs a complete doc structure overhaul. Maybe we can dedicate one of the upcoming symbolic numerics thrusday meetings to that and just get it done.

If one happen, drop me a note and I can come by as well

@isaacsas
Copy link
Member

isaacsas commented Apr 1, 2025

Note that if we deregister it does mean we will no longer have the future ability to trivially detect these functions in symbolic expressions.

Perhaps we should instead expand these functions by default when converting to other system types?

@isaacsas
Copy link
Member

isaacsas commented Apr 1, 2025

(Also, we should probably move these discussions to issues.)

@TorkelE TorkelE changed the title [WIP] Re-enable conservation laws for NonlinearSystems Re-enable conservation laws for NonlinearSystems Apr 1, 2025
@TorkelE TorkelE mentioned this pull request Apr 1, 2025
7 tasks
@isaacsas
Copy link
Member

isaacsas commented Apr 3, 2025

@TorkelE I had some merge issues on the tests when updating to master given the divergence between the PR and master now, please double check the tests are ok now.

@TorkelE
Copy link
Member Author

TorkelE commented Apr 3, 2025

Right, yeah, I probably should make a commit making these tests up to date (also running some of the new remake tests on NonlinearSystems as well, now when those works). When you are done with the update I will do one more update solely going through that the tests are good.

@isaacsas
Copy link
Member

isaacsas commented Apr 3, 2025

I think I'm done, but now I'm not sure if tests will fail due to them being out of sync with the code or something. I guess we shall see!

@TorkelE
Copy link
Member Author

TorkelE commented Apr 3, 2025

Yes! I will have a look at the tests.

@isaacsas
Copy link
Member

isaacsas commented Apr 8, 2025

@TorkelE if everything passes and the doc build looks ok I am happy to merge this (and will do so).

@isaacsas isaacsas merged commit 1e96aa5 into master Apr 8, 2025
9 of 13 checks passed
@isaacsas isaacsas deleted the fix_nlsys_conslaws branch April 8, 2025 23:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants