-
Notifications
You must be signed in to change notification settings - Fork 4
RateSystem #117
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
base: main
Are you sure you want to change the base?
RateSystem #117
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for working on this!
I very much like the idea of starting with the general mathematical formulation \dot x = f(x(t), lambda(t))
, and have made a suggestion of how this could be reflected more in the way a user would define a RateSystem
.
One thing I am unsure about is whether we just need a constructor function RateSystem
that returns either a CoupledODEs
or CoupledSDEs
, or whether we need an additional type in order to pass the necessary information about the rate system to the methods we want to add.
Maybe we can brainstorm the methods we would like to add here and think about whether they work with a CoupledODEs
or whether we need more.
src/RateSys.jl
Outdated
# t_i autonomous t_ni non-autonomous t_nf autonomous t_f | ||
# | | | | | ||
|
||
using DynamicalSystems |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be sufficient to use DynamicalSystemsBase
.
src/RateSys.jl
Outdated
|
||
# we write | ||
|
||
function RateSyst(tni,tnf,f,λ,p_λ,initvals) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion, these are quite many positional arguments, which makes it less user-friendly. What do you think of reorganising this in the following structure:
RateSystem(ds::ContinuousTimeDynamicalSystem, rate_protocol::function)
where ds
can be a CoupledODEs
or a CoupledSDEs
and rate_protocol
is your λ
function that determines how the parameters change over time. This function could have the structure
rate_protocol(u, p_lambda, t; t_start=0.0, t_end=Inf, kwargs...)
i.e. it would contain all the input arguments needed to define the rate forcing function. Behind the scenes, this would then create and return a new CoupledODEs
or CoupledSDEs
with the explicit time dependence specified by rate_protocol
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The naming here is just a suggestion, but generally I think we should give more descriptive names than just lambda
or tnf
(clarity over brevity!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds great, thanks Reyk! However, rate_protocol should not depend on u, in my opinion.
Hey, what's the status on this @raphael-roemer @ryandeeley ? |
Hello, Raphael and I met again last week to pick this up again. We agree that it would be nice to pass some used-defined
We are proposing that As I've written it, the positional arguments of
The first two arguments are hopefully clear. The argument One keyword argument of Note that there are two non-user-level functions I tested this proposed I've attached a |
Hey @ryandeeley, thanks for the update! Could you push your proposed changes to this branch? This is what git is designed for, and pushing here will automatically run the tests. (The idea is that this draft pull request is continuously developed until we are happy to merge it with the main branch) |
Yes, we can do that, although I only wrote this yesterday and haven't explicitly discussed it with Raphael yet, who is also drafting some |
…l script, created a dev folder, updated src/CriticalTransitions.jl file.
…itions.jl file to be in line with that of the main branch.
@raphael-roemer just a reminder you can run the docs locally with Liveserver.jl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi all, sorry this took a while, especially given I don't have that many comments. I'm sorry, if I knew I'd only have minor comments I'd done this sooner...
Primarily, let's please try to stick to a specific terminology, and I'd really like us to keep to the one that we have agreed in the video calls after a long back and forth: Δp, Δt
and r = Δp/Δt
is the effective rate.
The other point to discuss is the computational performance of this. It is really bad... :( I really think we should brainstorm how to improve it.
That being said, improving the performance of the source code is completely irrelevant of hte interface. So I'd recommend to implement the changes I've illustrated here, merge this PR asap and then update the other PR with the functions that actually use this type.
Meanwhiole I'll think of the performance of this and how to improve it.
If you don't like Also, @raphael-roemer please comment: how does |
@raphael-roemer can you please comment where in the source code you need the export of hte referrneced_sciml_model that you mentione din the email? I don't see it. |
Oh, I've missed this: nonauto_traj = trajectory(nonauto_sys.system, T, x0); this is not the way to go. There is a well defined API for what a You guys have done this work already when you created the |
I've just updated the benchmarks to use @btime nonauto_traj = trajectory($(nonauto_sys.system), T, x0);
@btime nonautoExpl_traj = trajectory($nonauto_sysexpl, T, x0); gives
this is absolutely massive, and the number of allocations shows very clearl problems in the implementation beyond the algorithm design. @raphael-roemer have you profiled your code and checked the basic components of julia optimization, (such as type instability, unecessary allocations, etc.), as e.g., mentioned here: https://modernjuliaworkflows.org/optimizing/ ? |
Thanks @Datseris for your review! So apart from the computational speed, do you find the interface design makes sense?
I wasn't in the video calls, so I didn't know about these discussions about the naming. I prefer the written out labels because
Happy to do this. One question: Is it a problem that the |
no, this is no problem at all. Essentially make sure that you subtype for f in (:step!, :set_state!, ....) # all api functions here
@eval DynamicalSystemsBase.$(f)(rs::RateSystem, args...; kw...)= $(f)(rs.system, args...; kw...)
end |
yes provided the changes I've outlined above are included! |
@raphael-roemer I'll work on this tonight unless you are already on it! |
@raphael-roemer Hey, if needed I am happy to jump in here, to optimize the code. Just let me know. Probably best to do this in a seperate PR |
This would be great - thank you! I am quite busy before Friday, so I can only work on this afterwards. |
src/r_tipping/RateSystem.jl
Outdated
) | ||
end | ||
|
||
function RateSystem( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you don't need anything fro mthe CoupledODEs
source code!!!! Just make sure you rpopagate all APi functions to the stored internal system field!!!!
for f in (:step!, :set_state!, ....) # all api functions here
@eval DynamicalSystemsBase.$(f)(rs::RateSystem, args...; kw...)= $(f)(rs.system, args...; kw...)
end
as for making it a "type" just subtype ContinuousTimeDynamicalSYstem this should "just work"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay thanks!
Heeeeeell yea baby. I now have a fully-automated, completely generic codebase for generating plots like in Ritchie et al that show when a system is tracking, returning but not tracking, or always tipping. Here is the plot: ![]() I've plotted it as two plots because I couldn't be bothered to make the subtraction of the 2 plots to have 1 plot, but the plot above is exactly the same as the plots in Ritchie et al 2023: ![]() The plots are for the simple 2D stommel model. The code is completely generic , fully agnostic of the dynamical system, and fully automated by utilizing I will contribute this method here once the p.s. you may want to join https://discourse.julialang.org/t/juliadynamics-monthly-meetings-round-2/132357 tomorrow because I'll organize a minisymposium in next years JuliaCon in Germany. |
This is great - I also think that the So I suggest I get this PR cleaned up, we merge, and then we can go from there with solving the speed issue/adding functionality. |
pidx::Int | ||
rc::RateConfig | ||
t0::Real | ||
forcing_start::Real |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type unstable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but perhaps it doesn't matter, as this is only a configuration, so ignore for now,.
but since this is only a configuration perhaps worth merging the RateCOnfig
inside this directly? just take its three fields and add them here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The less Types the better - always!
We would like to add functionality for Rate dependent forcing functionality and easy system creation of non-autonomous systems