Skip to content

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.

@ChrisRackauckas
Copy link
Member

This looks good for now. Though I think the path forward really is that a symbolic JumpSystem should have its variables as discretes and use the hybrid system stuff when possible. The reason is the purely discrete variables would then not be in the ODE when the two are mixed, that could be a big performance win. We probably need to sync about this.

@ChrisRackauckas ChrisRackauckas merged commit bf9fec8 into SciML:master Nov 7, 2024
31 of 39 checks passed
@isaacsas isaacsas deleted the add_odes_to_jumps branch November 7, 2024 11:39
@isaacsas
Copy link
Member Author

isaacsas commented Nov 7, 2024

Happy to chat about that at some point. Maybe we can talk at the next office hours.

That would require changes in JumpProcesses too presumably? I thought what we are setting up here — really in the other WIP PR — would be pretty similar to what one would normally write by hand.

@ChrisRackauckas
Copy link
Member

I don't think JumpProcesses.jl would need to change in order to accomodate it. It should just be a codegen change.

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