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

[WIP] Implement backward_all as a counterpart of forward_all #340

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

takuseno
Copy link
Contributor

Hi, @TE-AkioHayakawa san, @TE-TakuyaNarihira san!

I've implemented nn.backward_all as a counterpart of nn.forward_all.

It seems to work correctly. But I have some concerns about this.

  • implementation design looks a bit ugly because there are two backward_all functions including CgVariable and computation_graph.cpp.
  • unit tests have passed except a test of rewire_on

@AkioHayakawa-sony
Copy link
Member

AkioHayakawa-sony commented Feb 7, 2019

This is just a reminder.

  • move visit_function_backward outside from CgVariable class (nbla::visitfunction_backward)
  • add "grads" argument to backward_all
  • In python API, check the lengths of grads and variables and make these lengths same (by padding or slicing grads).
  • check the test code for rewire_on again (I will also check it out)

Anyway, almost all parts of your PR look good for me! Thank you for your contribution!

@TakuyaNarihira TakuyaNarihira added the release-note-core Auto-release; Core Engine label Apr 3, 2019
@takuseno takuseno changed the title Implement backward_all as a counterpart of forward_all [WIP] Implement backward_all as a counterpart of forward_all May 30, 2019
TE-StephenTiedemann pushed a commit that referenced this pull request Feb 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note-core Auto-release; Core Engine
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants