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

Weird non-performant scaling of add_edge! #468

Open
agchesebro opened this issue Oct 21, 2024 · 3 comments
Open

Weird non-performant scaling of add_edge! #468

agchesebro opened this issue Oct 21, 2024 · 3 comments

Comments

@agchesebro
Copy link
Member

The new add_edge! functionality has some puzzling performance issue with large graphs. For example, constructing a system of 300 Izhikevich neurons with the old syntax:

using Neuroblox, Random, Distributions, OrdinaryDiffEq

η_dist = Cauchy(0.12, 0.02)
N = 300
w = 1

blox = [IzhikevichNeuron(name=Symbol("Izh$i"), η=rand(η_dist), sⱼ=1.2308/N) for i in 1:N]

g = MetaDiGraph()
add_blox!.(Ref(g), blox)

@time for i ∈ 1:N
    for j ∈ 1:N
        add_edge!(g, i, j, Dict(:weight => w))
    end
end

takes ~0.2 seconds for the loop my computer, while with the new syntax:

g = MetaDiGraph()
add_blox!.(Ref(g), blox)

@time for i ∈ blox
    for j ∈ blox
        add_edge!(g, i => j; weight=w)
    end
end

takes ~16s. This scales with graph size (e.g., 200 is about the same for the first loop, but ~5s for the second).

@harisorgn
Copy link
Member

I am not very surprised by this. The new syntax does more work internally; it's the find_blox step that probably causes the time increase as it parses the graph for each blox in each pair. All in all, it sacrifices performance for convenience.

We might be able to make it faster, although it's mainly using functions from Graphs. A better solution which we discussed with @MasonProtter some time in the past is to have our own custom graph type and make searching over it more efficient.

@agchesebro
Copy link
Member Author

Thank you! Makes sense overall - for larger systems of neurons (~1k) I'll stick to the old syntax for performance then. One additional wrinkle: if you remove add_blox!.(Ref(g), blox) from the new syntax version, it throws a ExtraEquationsSystemException. So for example:

using Neuroblox, Random, Distributions, OrdinaryDiffEq

η_dist = Cauchy(0.12, 0.02)
N = 5
w = 1

blox = [IzhikevichNeuron(name=Symbol("Izh$i"), η=rand(η_dist), sⱼ=1.2308/N) for i in 1:N]

g = MetaDiGraph()
for i ∈ blox
    for j ∈ blox
        add_edge!(g, i => j; weight=w)
    end
end
@named sys = system_from_graph(g)

will say the system is unbalanced, but if you include the add_blox! before the edge connections it compiles just fine.

@harisorgn
Copy link
Member

Good catch @agchesebro , there was a duplication of a blox if the first connection is a self-connection. Fixed in #469

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

No branches or pull requests

2 participants