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_pass now clears nodes which will not be used #221

Merged
merged 3 commits into from
May 8, 2017

Conversation

Etragas
Copy link
Contributor

@Etragas Etragas commented May 8, 2017

Attempting to help issues mentioned in #219

This line removed any references to Array nodes which will not be revisited, since they have passed all their gradient information to their parents.

In the special case of the start_node, the gradients will not be lost, since setting outgrads[node] to None only deletes the reference, not the values. So as long as cur_outgrad holds on to the gradient reference all is fine.

On a test dataset, using a proprietary algorithm which can be roughly described as two RNN's feeding into eachother, I obtained the following gains in performance.

https://cloud.githubusercontent.com/assets/6620250/25814415/6a8f085a-33eb-11e7-9237-ce58fa09300a.png

In addition, I compared the numerical results of my change to the previous version, and they were identical.

@j-towns
Copy link
Collaborator

j-towns commented May 8, 2017

Your code is throwing this error on Python 3:

ERROR: Failure: TabError (inconsistent use of tabs and spaces in indentation (core.py, line 43))

should be easy to fix :)

@Etragas
Copy link
Contributor Author

Etragas commented May 8, 2017

Haha, sorry about that.
Should be good now.

@j-towns
Copy link
Collaborator

j-towns commented May 8, 2017

This looks good to me, being pernickety maybe it would be slightly cleaner to use

del outgrads[node]

or

outgrads.pop(node)

to completely remove node from the dictionary.

This would mean no left over/unnecessary reference to node (edit: does a dict contain references to its keys?).

@Etragas
Copy link
Contributor Author

Etragas commented May 8, 2017

I agree that it's cleaner, since it also drops the key as opposed to just setting it to None.

@mattjj mattjj requested a review from dougalm May 8, 2017 18:18
@mattjj
Copy link
Contributor

mattjj commented May 8, 2017

Thanks for this improvement, @Etragas and @j-towns! This is open-source software collaboration at its best.

I marked @dougalm as a reviewer because I think he's also modifying this part of the code, but on reflection I see no reason not to merge this change immediately, so long as he doesn't accidentally clobber it.

Thanks again for this!

@mattjj mattjj merged commit 3c7b69b into HIPS:master May 8, 2017
@mattjj
Copy link
Contributor

mattjj commented May 9, 2017

For posterity: see this related comment.

alexbw added a commit that referenced this pull request May 9, 2017
* 'master' of github.com:HIPS/autograd:
  Switched to incremental summation on the reverse pass to save memory in cases of high fan-out.
  backward_pass now clears nodes which will not be used (#221)
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

Successfully merging this pull request may close these issues.

3 participants