Skip to content

Conversation

@contradict
Copy link
Contributor

@contradict contradict commented Jun 19, 2020

Placing the logging callback inside the gradient block means Zygote will try to differentiate it. That gradient information is unlikely to be useful and likely to cause errors (especially about mutating arrays).

PR Checklist

  • Tests are added
  • Entry in NEWS.md
  • Documentation, if applicable
  • Final review from @MikeInnes or @dhairyagandhi96 (for API changes).

Placing the logging callback inside the gradient block means Zygote will try to differentiate it. That gradient information is unlikely to be useful and likely to cause errors (especially about mutating arrays).
Copy link
Member

@oxinabox oxinabox left a comment

Choose a reason for hiding this comment

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

While there are other options like ignore() do ...
this one has the key advantage that the PR is currently open and ready to be merged.

Thanks!

@CarloLucibello
Copy link
Member

CarloLucibello commented Jun 20, 2020

We could also add custom loop using pullback in this section. Something like

function my_custom_train!(loss, ps, data, opt)
  ps = Params(ps)
  for d in data
    train_loss, back = Zygote.pullback(() -> loss(d...), ps)
    gs = back(1f0)
    update!(opt, ps, gs)
  end
end

@contradict
Copy link
Contributor Author

This looks like an all-around nicer syntax, although it would need some more explanation (pullback is not mentioned anywhere else in the Flux documentation). If you want to switch to this syntax, I think I should close this PR and try again with this technique (after I read about it some myself).

@CarloLucibello
Copy link
Member

This looks like an all-around nicer syntax, although it would need some more explanation (pullback is not mentioned anywhere else in the Flux documentation). If you want to switch to this syntax, I think I should close this PR and try again with this technique (after I read about it some myself).

gradient is implemented in terms of pullback, see https://github.com/FluxML/Zygote.jl/blob/257dfb139d0cc3c88a87654c4412fa5b7947bddd/src/compiler/interface.jl#L52. pullback just returns the output of the function and a closure that performs backpropagation.

Instead of switching syntax, we can just add another example using pullback

It might no be clear what should be returned after adding code here, make the return explicit.
This example achieves the same effect with the pullback() method.
@contradict
Copy link
Contributor Author

OK, another try! How does this look?

@CarloLucibello
Copy link
Member

Looks good! I think we can add a pointer to Zygote's pullback documentation https://fluxml.ai/Zygote.jl/dev/adjoints/#Pullbacks-1 and we are done

@contradict
Copy link
Contributor Author

The link is in there, at the first mention of pullback on line 162. Thanks for looking this over!

@CarloLucibello
Copy link
Member

The link is in there, at the first mention of pullback on line 162. Thanks for looking this over!

right. Thanks for this PR!

bors r+

@bors
Copy link
Contributor

bors bot commented Jun 23, 2020

Build succeeded:

@bors bors bot merged commit e0a7e9b into FluxML:master Jun 23, 2020
@contradict contradict deleted the patch-1 branch June 24, 2020 03:50
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