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

fix: turn off parameter splitting for BifurcationProblem #1071

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AayushSabharwal
Copy link
Member

BifurcationProblem does not support symbolic indexing and as a result, MTK (somewhat hackily) just removes sys.index_cache in the BifurcationProblem constructor to force split = false behavior. However, with complete now reordering parameters, this breaks assumptions and makes analysis of these problems difficult.

In hindsight, BifurcationProblem should have just errored if the system was created with split=true, but changing that behavior is breaking. I will add a warning in MTK. For now, this should fix CI

Checklist

  • Appropriate tests were added
  • Any code changes were done in a way that does not break public API
  • All documentation related to code changes were updated
  • The new code follows the
    contributor guidelines, in particular the SciML Style Guide and
    COLPRAC.
  • Any new documentation only uses public API

Additional context

Add any other context about the problem here.

@isaacsas isaacsas requested a review from TorkelE October 1, 2024 07:07
@isaacsas
Copy link
Member

@AayushSabharwal just circling back to this now that Catalyst CI finally works again. Is this still needed? I think MTK's BifurcationKit extension was updated some since you submitted this to Catalyst?

@isaacsas
Copy link
Member

I don't see split = true being used there at all (in the tests for example).

@AayushSabharwal
Copy link
Member Author

It is still needed. complete with split = true (the default) reorders parameters. Since BifurcationProblem doesn't support symbolic indexing, the order matters for user accessibility.

@ChrisRackauckas
Copy link
Member

We can just make BifurcationProblem support SII? That seems like the better long term solution here.

@AayushSabharwal
Copy link
Member Author

I agree, this was written to get tests passing quickly. A quick look at BifurcationKit internals says it shouldn't be too difficult to support SII so I'll see what I can do.

@isaacsas
Copy link
Member

Tests seem to pass now in either case. MTK seems to explicitly not set this though: https://github.com/SciML/ModelingToolkit.jl/blob/1f2d9430cfc6b7ebe2044dfb1b9a078eac4af87e/ext/MTKBifurcationKitExt.jl#L152

So that needs updating I guess (until BifurcationKit is made to work with SII)?

@isaacsas isaacsas closed this Oct 23, 2024
@isaacsas isaacsas reopened this Oct 23, 2024
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.

3 participants