Skip to content
This repository has been archived by the owner on Apr 26, 2019. It is now read-only.

search: protect existing nodes during graph construction #36

Merged
merged 1 commit into from
May 15, 2015

Conversation

kortschak
Copy link
Member

No description provided.

@kortschak
Copy link
Member Author

ping @gonum/developers

@@ -370,7 +370,9 @@ func TestTarjan(t *testing.T) {
for i, test := range tarjanTests {
g := concrete.NewDirectedGraph()
for u, e := range test.g {
g.AddNode(concrete.Node(u))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this imply that the test is wrong, and thus a panic is more applicable?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it may be the case that an edge has been created to a node that has not yet been seen due to the test.g range loop. For example in the first Tarjan test case:

g: []set{
    0: linksTo(1),
    1: linksTo(2, 7),
    2: linksTo(3, 6),
    3: linksTo(4),
    4: linksTo(2, 5),
    6: linksTo(3, 5),
    7: linksTo(0, 6),
},

The first edge {0,1} creates the node 1, so by the time we get to adding node 1 explicitly, it is already there.

The API allows creation of both nodes implicitly during an AddDirectedEdge call and I will probably change these and others to that later, but here I want to be explicit until the API redesign has been more fully fleshed out (see #45 in particular).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@btracey Are you OK with this to be merged?

@btracey
Copy link
Member

btracey commented May 15, 2015

LGTM

@kortschak kortschak merged commit 6fa2aa4 into master May 15, 2015
@kortschak kortschak deleted the tarjan-test branch May 16, 2015 14:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants