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

JumpSystem cleanup #3180

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

JumpSystem cleanup #3180

wants to merge 5 commits into from

Conversation

isaacsas
Copy link
Member

@isaacsas isaacsas commented Nov 4, 2024

As a first step before working on making JumpSystems support coupled ODEs/SDEs, I wanted to clean up the code a bit. This

  1. Adds support for collect_vars! for JumpSystems.
  2. Cleans up the constructor to be more in line with how the code works for other system types. If at some point refactoring of common code in constructors is done it should now be easier to see what is different in JumpSystems.

Note that JumpSystems would still need some more work to support hierarchical modeling (i.e. sub-systems and scoping).

@isaacsas
Copy link
Member Author

isaacsas commented Nov 4, 2024

My plan is to next make the ArrayPartition that stores the jumps also have a component for equations, which will be used to generate an ODESystem during ODEProblem(::JumpSystem, ...) (allowing a coupled ODE-jump system).

Copy link
Member

@AayushSabharwal AayushSabharwal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Could you check if removing the if !(sys isa JumpSystem) here now works? It was added because collect_vars! didn't work

@isaacsas
Copy link
Member Author

isaacsas commented Nov 4, 2024

Will update later for that. I think we need a dispatch on the trait I added in collect_scoped_vars!.

@isaacsas
Copy link
Member Author

isaacsas commented Nov 4, 2024

Should be updated now.

@isaacsas
Copy link
Member Author

isaacsas commented Nov 4, 2024

Lots of failures here, but as far as I can tell none are related to this PR. I formatted with JuliaFormatter v1.0.62 so not sure why that is failing either. Locally it reports there is nothing to format when run on the repo.

@isaacsas
Copy link
Member Author

isaacsas commented Nov 5, 2024

@ChrisRackauckas I think this is good to merge now -- the test failures all seem unrelated.

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.

2 participants