Stochsystem overhaul #76
Conversation
|
I've taken a quick look at this and have three comments:
|
The documentation of DynamicalSystems.jl says that recommend using ODEProblem when using callbacks. |
|
We should only merge once the CI, Dowgrade and docs are working. Will take a look tonight :) |
|
The
Can check later what the issue is. Also, |
| @@ -30,7 +30,7 @@ The minimization can be performed in blocks to save intermediate results. | |||
| If `save_info`, returns `Optim.OptimizationResults`. Else, returns only the optimizer (path). | |||
| If `blocks > 1`, a vector of results/optimizers is returned. | |||
| """ | |||
| function min_action_method(sys::StochSystem, x_i::State, x_f::State, N::Int, T::Real; | |||
| function min_action_method(sys::CoupledSDEs, x_i::SVector, x_f::SVector, N::Int, T::Real; | |||
There was a problem hiding this comment.
I think it is best to not force to user to use Svector
src/CriticalTransitions.jl
Outdated
| @@ -50,6 +50,7 @@ export equilib, fixedpoints, basins, basinboundary, basboundary | |||
| export simulate, relax | |||
| export transition, transitions | |||
| export fw_integrand, fw_action, om_action, action, geometric_action | |||
| export div_drift | |||
There was a problem hiding this comment.
To keep the namespace minimal, we should limit the amount of functions we export. I don't think we need to export div_drift
There was a problem hiding this comment.
I think it would be convenient to export because it is quite central to the Onsager-Machlup action. Of course one can compute it pretty easily. We could also consider not exporting fw_integrand
There was a problem hiding this comment.
But a user would never call it in a usual workflow right? If they want to call they can just import the function in the namespace or calll it with Criticaltransition.div_drift
|
We need to update the docs. |
|
dev, examples and ext have still to be updated |
| @@ -14,7 +14,7 @@ As this module is not published yet, there are two ways to access it: | |||
| ## Basic usage | |||
| The general workflow of `CriticalTransitions` essentially follows two steps: | |||
|
|
|||
| 1. Define your system (see [Define a CoupledSDE](@ref)) | |||
| 1. Define your system (see [Define a CoupledSDEs system](@ref)) | |||
There was a problem hiding this comment.
This is a reference to a tag. So if we change it here. It should be changed everywhere
| @@ -16,6 +16,11 @@ using Symbolics | |||
| using Optim, Dierckx | |||
| using Printf, DrWatson, Dates, Statistics | |||
|
|
|||
| # Define custom types | |||
| Parameters = Union{Vector{Any}, Nothing}; | |||
There was a problem hiding this comment.
Adding these types also means we should use it. In general, we try to not restrict to use specific types (generic programminf), e.g,, the parameter type in SciML ecosystem can be a tuple, array, dictionary, etc. I think we should follow what JuliaDynamics does.
There was a problem hiding this comment.
I agree, I just re-added these custom types because some functions are still dependent on them, which caused an error when they were removed. In the long-run we should follow the style conventions, absolutely.
|
Work on #75, #42 and #22