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

backward member implementation question #68

Open
strisys opened this issue Apr 20, 2024 · 1 comment
Open

backward member implementation question #68

strisys opened this issue Apr 20, 2024 · 1 comment

Comments

@strisys
Copy link

strisys commented Apr 20, 2024

Why can't this function simply be implemented as follows? Am I missing something? We are dealing with a composite structure.

  def backward(self, is_first=True):
    if (is_first == True):
      self.grad = 1.0

    self._backward()

    for c in self._prev:
      c.backward(False)
@daghanerdonmez
Copy link

daghanerdonmez commented Jun 20, 2024

The current implementation first does a reverse topological sort on all nodes (No parent comes before its child. For example x and y are parents of the expression (x+y). ) Then it calls _backward() in that order. So when we call the function _backward() for a node, we guarantee that its .grad property has its final (and correct) value and will not change in the future.

Consider the following example:

  • a = 2
  • b = 3
  • c = a + b
  • d = a * b
  • e = c + d
  • f = c * d
  • y = e + f

Your method will give an incorrect result for this example. I won't show it step by step as it looks too long and complex in text but you can calculate it with hand to see the problem. The problem will occur in the following way:

  • You set y.grad = 1 (correct)
  • You set e.grad = 1 (correct)
  • Then, because you are using recursion, you will go into the node "c".
  • You will set c.grad = 1 (This is not the complete/correct result obviously. This is only the part that comes from e. You will also need to add a +d term when you are calculating the gradient through f. So, as you can verify via calculus dy/dc = 1+d)
  • Now, again using recursion you will continue to the "a" node.
  • Now you will set a.grad = 1 (This is incorrect, you have calculated it thinking that dy/dc = 1. But the correct value was 1+d.)

I hope I could explain the problem clearly. The problem with your method is basically using the wrong, temporary .grad values of a node to calculate the .grad of its children.

If you are confused by the whole node and graph logic, watching this video would help you understand the structure more clearly as @karpathy imagined it.

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