Switch to Flux with zygote #19
Conversation
|
@ablaom I'm not sure if it's a good time to switch to Zygote. If it isn't we'll probably let this PR be idle. Else, we can merge this. |
|
Many thanks for this! @ayush-1506 I have asked for feedback on this question here (slack): https://julialang.slack.com/archives/C7LFJTXV5/p1585591105130500 and copied you in. So far, the reaction is mixed. Let's hold out at least until my code review is finished, thanks! |
|
I wonder if it is too much trouble to support both Flux streams. We could put some magic into the Thoughts? |
|
@ablaom We could do that, but I'm not sure if tracker/zygote makes a lot of difference to the user. I can't think of a reason why someone using, say, NeuralNetworkClassifier would like to use Zygote instead of Tracker (or vice-versa), since they directly don't need to deal with the TrackedValues or gradients. For other more complicated applications, maybe it makes sense. The main advantage of switching to Zygote is so that we keep using the latest Flux and don't get stuck with older versions as Flux moves forward. Let me know what you think. |
|
Well, the problem, as I understand it is that Zygote still has issues, according to one person on the slack discussion I mentioned. |
|
I guess that's true. There's a lot of activity on the zygote issue tracker. I'm more oriented towards using Tracker at the moment (since it gets the job done in most cases) and switching to Zygote when we think it's a good time. If you think it's better to add an option, let me know. |
|
@ayush let's switch to zygote, ie bump [compat] Flux="^10.4" and do the necessary refactoring. You happy to re-activate this? |
|
Opening a new PR for this |
Involves just a few small changes (Since the overall API hasn't changed much). Tests pass locally. Since our gradients aren't tracker types, we don't need to get the
.dataattribute anymore.