Bug fix: properly partition networks containing one-sided gap-junction connections #1774

noraabiakar · 2021-11-22T15:28:24Z

The current partition_load_balance function assumes that gap-junction connections are always double-sided (i.e. if gid x has a gap-junction connection from peer gid y, then gid y must also have a gap-junction connection from peer gid x). This used to be a requirement that was checked prior to #1682. Since then, single-sided gap-junctions are in principle allowed, but partition_load_balance still operates under the bidirectional gap-junction connection assumption resulting in some gids being present in multiple cell-groups.
This PR modifies the partition_load_balance function to do the following:

On each rank, generate a gj_connection list per cell. (gj_connection list is a vector of gids that have an outgoing connection ending at the cell under consideration).
On each rank, gather all the gj_connection lists for each cell in the network.
On each rank, modify the global list of gj_connections in the network to make all gj_connections bidirectional.
On each rank, use the global list of gj_connections in the network to partition the graph.

Fixes #1767.

bcumming · 2021-11-23T15:50:24Z

bors try

noraabiakar · 2021-11-26T10:09:58Z

bors try

bors · 2021-11-26T10:09:59Z

try

Already running a review

bors · 2021-11-29T02:00:12Z

try

Build failed:

ci/gitlab/gitlab.com

noraabiakar · 2021-11-29T10:28:01Z

bors try

bors · 2021-11-29T11:06:13Z

try

Build failed:

ci/gitlab/gitlab.com

noraabiakar · 2021-12-02T09:30:47Z

bors try

bors · 2021-12-02T09:41:43Z

try

Build succeeded:

ci/gitlab/gitlab.com

thorstenhater · 2021-12-15T09:06:41Z

Hi @noraabiakar,
could you elaborate why that bug happened a bit more,
would be helpful before diving into the code.

noraabiakar · 2021-12-15T11:02:09Z

Let's say we have a system of 2 cells and cell 0 has an incoming gap junction connection from cell 1 (but no outgoing connection). This is not supported at the moment but it won't raise any exceptions.

Our connected components algorithm assumes that all gap junction connections are bidirectional and will come up with the following cells groups: {0, 1} and {1}, which is incorrect. What this PR does is change all unidirectional gap-junctions to bidirectional gap-junctions prior to calling the connected components algorithm which will then do the correct thing again, generating only one cell groups {0,1}.

bcumming

Looks good. Just a few suggestions to improve performance

bcumming · 2022-01-05T09:54:59Z

arbor/communication/mpi.hpp

+    std::vector<int> counts_internal, displs_internal;
+
+    // Vector of individual vector sizes
+    std::vector<int> internal_sizes(values.size());


int is a narrowing cast from size_t, so I would suggest using unsigned long.

bcumming · 2022-01-05T09:57:26Z

arbor/partition_load_balance.cpp

+
+    // Generate a local gj_connection table.
+    // The table is indexed by the index of the target gid in the gid_part of that domain.
+    // If gid_part[domain_id] = [a, b[; local_gj_connection of gid `x` is at index `x-a`.


The formatting of the braces is wonky: should it be [a, b]?

It's intended to show that b in not in the range.

A "half open range" [a, b) == [a, a+1, a+2, ..., b-1]?

bcumming · 2022-01-05T09:59:42Z

arbor/partition_load_balance.cpp

+            local_gj_connection_table[gid-dom_range.first].push_back(c.peer.gid);
+        }
+    }
+    // Sort the inner vectors of gj_connections.


Change comment to give more context, e.g. sort the gj connections of each local cell

bcumming · 2022-01-05T10:03:06Z

arbor/partition_load_balance.cpp

+            // If gid is not in the peer connection table insert it such that
+            // the vector remains sorted.
+            auto it = std::lower_bound(peer_conns.begin(), peer_conns.end(), gid);
+            if (it == peer_conns.end() || *it != gid) {


This has the potential to be really expensive both in terms of time, and in memory if there are many insertions.
How about looping over once to count how many insertions have to be made (if any), allocate a new vector if need be, then do the insertions?

I'm not sure I understand what you mean, but I changed the implementation.

I might have misunderstood what was being done here, but it looks like:

search through a sorted vector for a value

if the value is not found, insert it such that the vector is still sorted

There are two performance issues here:

inserting in a vector will require n/2 copies to move the values after the inserted value out of the way

insertion may also require reallocation of more memory, and a copy of the whole vector contents

I think the new implementation is better. The vector doesn't need to be sorted in the end.

Check that a gid is available in the global_gj_connection_table[peer] vector, if not, insert it into an unordered_set.

Append the values of the unordered_set onto the global_gj_connection_table[gid] vectors.

bcumming · 2022-01-05T11:16:24Z

arbor/partition_load_balance.cpp

+                    auto conns = global_gj_connection_table[element];
+                    for (const auto& peer: conns) {


To avoid making a copy of global_gj_connection_table[element], which is a std::vector, and looking up peer twice in the visited set, consider the following:

for (const auto& peer: global_gj_connection_table[element]) { if (visited.insert(peer).second) { q.push(peer); } }

I think that was supposed to be const auto& conns = global_gj_connection_table[element]. I will implement your suggestion anyway.

I actually meant what I proposed: there is no need to create the const auto& cons, though whether you do or don't is a matter of personal preference.

noraabiakar · 2022-01-05T12:18:55Z

bors try

bors · 2022-01-05T12:26:25Z

try

Build failed:

ci/gitlab/gitlab.com

bcumming · 2022-01-05T13:23:08Z

arbor/partition_load_balance.cpp

-
+    // Append the missing peers into the global_gj_connections table
+    for (unsigned i = 0; i < global_gj_connection_table.size(); ++i) {
+        std::move(missing_peers[i].begin(), missing_peers[i].end(), std::back_inserter(global_gj_connection_table[i]));


are the entries in global_gj_connection_table[i] supposed to be sorted?
The loop above assumes that they are, which enables std::binary_search/std::lower_bound, however back_inserter to add missing values is not guaranteed to maintain sorted values.

noraabiakar added 7 commits November 19, 2021 16:31

WIP

d5a6293

add unit test; fix bug

329316f

add unit tests

ab3b445

small fix

7f69726

cleanup

c604b7b

add unit test

feb9c4f

add some diagrams to comments

5f168cf

bors bot added a commit that referenced this pull request Nov 27, 2021

Try #1774:

2f89f0a

bors bot added a commit that referenced this pull request Nov 29, 2021

Try #1774:

9f040a1

bors bot added a commit that referenced this pull request Dec 2, 2021

Try #1774:

dc2da97

noraabiakar assigned thorstenhater Dec 14, 2021

noraabiakar requested a review from thorstenhater December 15, 2021 08:26

bcumming requested changes Jan 5, 2022

View reviewed changes

Address PR comments

c56b11a

bors bot added a commit that referenced this pull request Jan 5, 2022

Try #1774:

b087887

bcumming reviewed Jan 5, 2022

View reviewed changes

Address PR comments

30e22a4

bcumming approved these changes Jan 6, 2022

View reviewed changes

bcumming merged commit 0636bbf into arbor-sim:master Jan 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug fix: properly partition networks containing one-sided gap-junction connections #1774

Bug fix: properly partition networks containing one-sided gap-junction connections #1774

noraabiakar commented Nov 22, 2021

bcumming commented Nov 23, 2021

noraabiakar commented Nov 26, 2021

bors bot commented Nov 26, 2021

bors bot commented Nov 29, 2021

noraabiakar commented Nov 29, 2021

bors bot commented Nov 29, 2021

noraabiakar commented Dec 2, 2021

bors bot commented Dec 2, 2021

thorstenhater commented Dec 15, 2021

noraabiakar commented Dec 15, 2021

bcumming left a comment

bcumming Jan 5, 2022

bcumming Jan 5, 2022

noraabiakar Jan 5, 2022

bcumming Jan 5, 2022

bcumming Jan 5, 2022

bcumming Jan 5, 2022

noraabiakar Jan 5, 2022

bcumming Jan 5, 2022

noraabiakar Jan 5, 2022

bcumming Jan 5, 2022

noraabiakar Jan 5, 2022

bcumming Jan 5, 2022

noraabiakar commented Jan 5, 2022

bors bot commented Jan 5, 2022

bcumming Jan 5, 2022

		auto conns = global_gj_connection_table[element];
		for (const auto& peer: conns) {

Bug fix: properly partition networks containing one-sided gap-junction connections #1774

Bug fix: properly partition networks containing one-sided gap-junction connections #1774

Conversation

noraabiakar commented Nov 22, 2021

bcumming commented Nov 23, 2021

noraabiakar commented Nov 26, 2021

bors bot commented Nov 26, 2021

try

bors bot commented Nov 29, 2021

try

noraabiakar commented Nov 29, 2021

bors bot commented Nov 29, 2021

try

noraabiakar commented Dec 2, 2021

bors bot commented Dec 2, 2021

try

thorstenhater commented Dec 15, 2021

noraabiakar commented Dec 15, 2021

bcumming left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noraabiakar commented Jan 5, 2022

bors bot commented Jan 5, 2022

try

Choose a reason for hiding this comment