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

Topological sort - bug #67

Open
gordicaleksa opened this issue Apr 19, 2024 · 4 comments
Open

Topological sort - bug #67

gordicaleksa opened this issue Apr 19, 2024 · 4 comments

Comments

@gordicaleksa
Copy link

It's a nit that won't matter most of the time but the topo sort implementation doesn't work in case you have cycles in the graph.

i.e. there is a hard assumption you're operating over a DAG.

@Narasimhareddy-B
Copy link

A directed acyclic graph is a directed graph that has no cycles. A vertex v of a directed graph is said to be reachable from another vertex u when there exists a path that starts at u and ends at v. As a special case, every vertex is considered to be reachable from itself (by a path with zero edges).

@Jet-lag
Copy link

Jet-lag commented Jul 4, 2024

It's a nit that won't matter most of the time but the topo sort implementation doesn't work in case you have cycles in the graph.

i.e. there is a hard assumption you're operating over a DAG.

It's correct. On the other hand, if it is a Dag, can we simply write the backward function as

def backward(self):  
    self._backward()  
    for child in self._prev:  
      child.backward()

@pkulijing
Copy link

pkulijing commented Jul 16, 2024

It's a nit that won't matter most of the time but the topo sort implementation doesn't work in case you have cycles in the graph.
i.e. there is a hard assumption you're operating over a DAG.

It's correct. On the other hand, if it is a Dag, can we simply write the backward function as

def backward(self):  
    self._backward()  
    for child in self._prev:  
      child.backward()

No. Your implementation is essentially a DFS, while topological sort requires a BFS. For this simple case your code could be wrong:

b = 2*a
c = a + b

with topological sort, it's guaranteed that the back propagation goes in the order of c -> b -> a. With your code, the order could be c -> a -> b, which is wrong.

@conscell
Copy link

@gordicaleksa
Could you please provide an example when the computational graph is not a DAG?

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

5 participants