-
Notifications
You must be signed in to change notification settings - Fork 13
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
Heterogeneous lags for networks with delay #104
Conversation
The history function is now wrapped, such that it always has access to the global parameters and the global index ranges of the corresponding vertices. As a consequence a call to
This should provide reasonable support for heterogeneous delays. However due to the interpolation bug mentioned above even medium-sized heterogeneous systems take very long to solve. |
.github/workflows/tests.yml
Outdated
@@ -12,7 +12,7 @@ jobs: | |||
fail-fast: false | |||
matrix: | |||
version: | |||
- '1.6' | |||
- '1.8' |
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.
- '1.8' | |
- '1.6' | |
- '1.8' |
we shouldn't skip testing LTS?
examples/Project.toml
Outdated
@@ -18,4 +19,4 @@ StochasticDiffEq = "789caeaf-c7a9-5a7d-9973-96adeb23e2a0" | |||
Test = "8dfed614-e22c-5e08-85e1-65c5234f0b40" | |||
|
|||
[compat] | |||
NetworkDynamics = "0.5" | |||
NetworkDynamics = "≥ 0.5" |
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.
Are the examples really compatible with all breaking versions since 0.5 and also in the future? Maybe we should remove the compat all together in this file as this might be misleading...
src/ComponentFunctions.jl
Outdated
@@ -285,7 +285,7 @@ Here `dv`, `v`, `p` and `t` are the usual ODE arguments, while | |||
|
|||
- `dim` is the number of independent variables in the edge equations and | |||
- `sym` is an array of symbols for these variables. | |||
- `coupling` is a Symbol describing if the EdgeFunction is intended for a directed graph (`:directed`) or for an undirected graph (`{:undirected, :fiducial}`). `:directed` is intended for directed graphs. `:undirected` is the default option and is only compatible with SimpleGraph. in this case f! should specify the coupling from a source vertex to a destination vertex. `:fiducial` lets the user specify both the coupling from src to dst, as well as the coupling from dst to src and is intended for advanced users. | |||
- `coupling` is a Symbol describing if the EdgeFunction is intended for a directed graph (`:directed`) or for an undirected graph (`{:undirected, :fiducial}`). `:directed` is intended for directed graphs. `:undirected` is the default option and is only compatible with SimpleGraph. in this case f should specify the coupling from a source vertex to a destination vertex. `:fiducial` lets the user specify both the coupling from src to dst, as well as the coupling from dst to src and is intended for advanced users. |
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.
Only tangentially related to this PR, but this docstring should really mention how fiducial
work. I.e. you specify the dimensionality N
, you get passed a 2N
state vector where the first N
elements will be presented to the dst and the second N
elements will be presented to src
src/ComponentFunctions.jl
Outdated
dim % 2 == 0 ? nothing : error("Fiducial edges are required to have even dim. | ||
The first dim args are used for src -> dst, | ||
the second for dst -> src coupling.") |
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 old version with &&
is more idomatic than using the ternary operator i'd say. Also, multi line strings are possible in Julia, but only from 1.7
onwards if I remeber correctly. Therefore, the "
and ,
solution is better because it does not include all of the whitespace in the literal string
src/NetworkDiffEq.jl
Outdated
@@ -111,15 +120,14 @@ end | |||
# struct for both cases | |||
|
|||
#@Base.kwdef struct NetworkDE{G, GDB, elV, elE, TUV, TUE, Th<:AbstractArray{elV}} | |||
Base.@kwdef struct NetworkDE{G,GDB,elV,elE,TUV,TUE,Th<:Union{AbstractArray,Nothing}} | |||
@Base.kwdef struct NetworkDE{G, GDB, elV, elE, TUV, TUE} |
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.
most styleguides ommit those spaces afaik
Co-authored-by: Hans Würfel <[email protected]>
Co-authored-by: Hans Würfel <[email protected]>
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.
i agree with all your suggestions. some of them had already been adressed in main but accidentally leaked from this 8 month old branch. thanks for the review!
Delays are now implemented by passing the history function to the components. This allows to define several different lags at every network component and to directly control these lags via the parameter tuple
p = (vertexp, edgep)
. Hence, there is no need for the 3-tuple syntax anymore and it is removed from the codebase. This refactor is important, since previously only homogeneous lags were possible.For fast solving with heterogeneous parameters we rely on efficient interpolations via
h(p,t, idxs=idxs)
. However, due to a bug in the Julia compiler, some type instability occurs and the performance suffers when using theidxs=idxs
interface (SciML/DelayDiffEq.jl#218).For a diffusion on a small complete graph with 10 vertices and with homogeneous lags i observed 10x slower solving than in the previous versions which used a mutating history function
h(buffer, p,t)
, seetest/delay_test.jl
.I am hesitant to merge this PR before the bug has been resolved.