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

refactor: store discrete portion in BlockedArray for efficiency, better handle event variables #2945

Merged
merged 4 commits into from
Aug 20, 2024

Conversation

AayushSabharwal
Copy link
Member

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.

@AayushSabharwal AayushSabharwal marked this pull request as ready for review August 12, 2024 12:32
@AayushSabharwal AayushSabharwal force-pushed the as/callback-split branch 2 times, most recently from ef3da8b to a29966b Compare August 12, 2024 13:34
@isaacsas
Copy link
Member

This looks like a really great addition! I'm just wondering is there any simple way to control saving of the discrete values (and if so could you add it to your nice doc example)?

@AayushSabharwal
Copy link
Member Author

What do you mean "control" saving?

@isaacsas
Copy link
Member

isaacsas commented Aug 12, 2024

My understanding is that the past values of parameters are now being saved if modified via a callbacks as shown in your doc example. Is there a way to disable such saving. (I.e. the current behavior where only the current value is stored.)

@AayushSabharwal
Copy link
Member Author

I see. Yeah, if the parameter is not time-dependent. @parameters c instead of @parameters c(t), it won't be saved. I'll document that better.

@isaacsas
Copy link
Member

Just to clarify, if I have a parameter c that I change in a DiscreteCallBack, even just once, with this PR its history will now get saved right (whereas previously no history was saved)? If so, what I was asking was if there could be a documented way to easily turn off this saving process.

@isaacsas
Copy link
Member

The idea being that in things like jump problems memory allocations can have a performance impact, so one often wants to turn off all saving beyond what exactly is needed. (This may not seem critical at the level of one simulation, but it can add up when running hundreds of thousands of simulations or more for sampling purposes.)

@AayushSabharwal
Copy link
Member Author

Only if it's declared to have a time dependency (@parameters c(t)). If it's declared as @parameters c, it won't be saved.

@isaacsas
Copy link
Member

Got it, thanks for clarifying! My apologies for not understanding that.

@AayushSabharwal
Copy link
Member Author

Right now I don't think anyone does this (please correct me if I'm wrong) so it shouldn't save by default for most simulations.

@isaacsas
Copy link
Member

isaacsas commented Aug 12, 2024

Correct, and setting it up to save only if you make it explicitly time-dependent seems like a nice way to handle this issue.

@isaacsas
Copy link
Member

This example in Catalyst https://docs.sciml.ai/Catalyst/stable/model_simulation/examples/periodic_events_simulation/#periodic_event_simulation_plotting_light would actually be a perfect use case for the new functionality, so I'll update that tutorial once this is released.

@AayushSabharwal AayushSabharwal force-pushed the as/callback-split branch 3 times, most recently from b424b73 to ecd061d Compare August 19, 2024 05:18
@AayushSabharwal
Copy link
Member Author

Requires JuliaSymbolics/SymbolicUtils.jl#634

@ChrisRackauckas ChrisRackauckas merged commit 06445fa into SciML:master Aug 20, 2024
21 of 23 checks passed
@AayushSabharwal AayushSabharwal deleted the as/callback-split branch August 20, 2024 12:17
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