First attempt at Yarin Gal dropout for LSTM #182
Conversation
|
If you want to avoid linking to cg maybe you can do something like using a flag Then in Not sure if that's considered less ugly but at least you don't need an additional pointer |
|
I need the CG to create the dropout masks. |
|
Ah! Got it now. Not sure it's less ugly.. |
|
Another solution would be to overload the start_new_sequence interface to accept cg as a parameter. This could be useful for other stuff as well |
| @@ -45,12 +45,20 @@ struct LSTMBuilder : public RNNBuilder { | |||
| Expression set_h_impl(int prev, const std::vector<Expression>& h_new) override; | |||
| Expression set_s_impl(int prev, const std::vector<Expression>& s_new) override; | |||
| public: | |||
| bool gal_dropout; | |||
There was a problem hiding this comment.
Maybe I missed this, but it looks like this isn't initialized to a default value anywhere?
There was a problem hiding this comment.
line 85 of lstm.cc -- but yeah this is a bit hacky as it disables the previous dropout behavior.
which leads me to the next question: should we disable the previous dropout behavior (and remove it from the code) or keep supporting both?
There was a problem hiding this comment.
Hmm, this does seem a bit hacky. I think we need to decide whether we're going to disable the previous dropout behavior or not. If we'll disable we can simplify the code, and if we need both it'll have to be a flag doing the intuitive switch between the two dropout methods. I'd prefer the simplicity of the former, but maybe we need empirical results. I can try to run some experiments testing the new method if that'd be helpful?
There was a problem hiding this comment.
if you have some ready-to-run benchmarks you trust, that'd be great!
| // YG init dropout masks for each layer | ||
| masks.clear(); | ||
| for (unsigned i = 0; i < layers; ++i) { | ||
| std::vector<Expression> masks_i; |
There was a problem hiding this comment.
Indents should be two spaces (throughout)
|
So I tested this and the results look OK on training, but they're horrible on test. During test time we don't apply dropout, and unlike the original "dropout" node (which compensates for this by multiplying the scale of the remaining nodes by 1/dropout_rate), this is just using random_bernoulli masks without this scaling. I haven't read the original paper closely enough to know how they fixed this discrepancy, but I imagine that we'll have to do this here as well. |
|
Right! good catch. I now follow Gal's implementation and if we are in "test_mode" we scale by dropout rate (which is actually the rate of units we keep). This means you should set builder.test_mode = true before testing. |
|
Two thoughts:
On Tue, Nov 22, 2016 at 9:56 AM, Yoav Goldberg [email protected]
|
|
What do we do for test time in dropout in RNNs? Or do we just scale the weights already at train time so it's not needed in test? (if it's the second, I'd rather do the testing on current version, and only then switch to that solution and verify they are indeed equivalent) BTW, are the exact equations that the dynet LSTM implement written somewhere (not in code)? Re Zoneout: sure I can give it a shot, but we really need to figure out how to support multiple regularization strategies in the code, at least at the API level. This really becomes messy once we have more than one or two. (perhaps duplicate the builder and have diff builder for each dropout variant?) |
|
API problems and zoneout non-withstanding, I'll take a look at this now with the current API. |
|
P.S., thanks to @pmichel31415 the LSTM equations are "documented" here: #154 |
|
FYI, my initial tests on an attentional model over the "train-big" set in my nmt-tips repository (https://github.com/neubig/nmt-tips) showed results that look promising for Gal, but perhaps too bad for traditional dropout. I'll investigate this a bit more. Standard 0.0: dev=7.57288 Gal 0.0: dev=7.96906 Also, one thing that caught me here was that in the new implementation the "dropout_rate" variable seems to actually to have the semantics of "retention rate," which is pretty confusing (the current implementation in master has the standard semantics). We should probably do a global search and replace for everywhere where the dropout_rate is used, and replace it with 1-dropout_rate, right? |
|
yes, I agree that the "retention rate" semantics is confusing -- the place to fix this would be in start_new_sequence_impl. Setting 1-dropout_rate everywhere in that function should work. note that if you are using this branch as is, then you effectively have only Gal dropout, so you need to test the regular dropout from the master branch if you don't do so already. |
|
Yes, my "standard" numbers reflect the master branch. I'll tell you when I've taken a closer look at why the standard numbers are getting counter-intuitive results. |
|
OK, here are the results for two runs (the dropout rate listed in the results is the actual dropout rate, not retention rate). Looks like Gal dropout is convincingly better almost always, so I'm in favor of switching the default to Gal dropout. I do really like the ability to add dropout+scaling at training time, and just use the parameters at testing time though, so hopefully we can make that work here too. ** Run 1 ** Run 2 |
| @@ -80,8 +82,44 @@ void LSTMBuilder::new_graph_impl(ComputationGraph& cg) { | |||
| // layout: 0..layers = c | |||
| // layers+1..2*layers = h | |||
| void LSTMBuilder::start_new_sequence_impl(const vector<Expression>& hinit) { | |||
| if (dropout_rate) { gal_dropout = true; } else { gal_dropout = false; } | |||
There was a problem hiding this comment.
I'm not 100% clear on this but I think this interacts counterintuitively with the inverted dropout semantics.
My experimental observation is that if I do rnn.set_dropout(0.1) my training loss doesn't go down at all (I assume because this means 10% retention rate) but if I do rnn.set_dropout(0.0) it converges (dropout is completely off), I think because this if doesn't trigger.
I imagine this won't be an issue when this is merged, if the old behaviour is not kept, but I figured this comment might be useful to others trying to use the code in this PR.
|
Ok. Unless there are objections, I will proceed with an update that:
|
|
Yes, that'd be great. And also the trivial reversing of the polarity of the dropout rate to make sure it's not the retention rate. |
* Remove previous dropout behavior. * set_dropout now indicates dropout_rate, not retention rate. * weights are scaled when dropout is applied, so no scaling is needed at test time (just set dropout_rate = 0.0)
|
@neubig LSTMBuilder now has Gal Dropout with scaling at train time and nothing at test time. I did some minimal testing of this locally, but would be good if you could re-run your prev benchmark to see if we obtain the same results. Something that bothers me on the scaling-at-train-time behavior: perhaps we should not scale according to the bernoulli parameter, but rather according to the number of 1s that actually appear in the bernoulli vector? this seems more correct. |
|
OK, tests are started. |
|
@yoavg Also, perhaps instead of calculating random_bernoulli() and then doing scaling, maybe we can just use the dropout() expression? I think this is basically the same thing, so I'll do another test doing this. Regarding how to scale, I agree that the actual number of 1s might be better, but this complicates the code a bit by requiring two passes (one to calculate the random variables and count them, then another to do the scaling), and in the limit of vector size they will be the same, so I'm not sure whether testing this extensively is worth the effort. |
|
@neubig Wait! there's a stupid bug. let me fix and push. |
|
The problem with using the dropout expression is that the same mask should be used for the entire sequence. Re the scaling according to number of ones, let me try and implement this also and we can compare what works better. |
|
@neubig (fixed the stupid scaling bug) |
|
@yoavg OK, rerunning this. Also, I added the ability to scale random bernoulli variables: 71fc893 This allows us to do scaling in a single operation without adding the extra multiplication. I'm testing a version of this commit with this change as well, and will contribute it if it's doing the same thing. |
|
@neubig great. I also added a version with the scaling according to the number of 1s under the bernoulli_scaling branch. |
|
Hmm, so my tests are almost finished, but inverted dropout doesn't seem to be working as well. I need to look into this a little more, and also test out the idea of using the actual number of 1s in the bernoulli samples. *** Run 1 *** Run 2 |
|
the bernoulli_scaling branch has implementation of using the actual number of 1s in the bernoulli samples. |
|
So I ran more experiments with the bernoulli_scaling branch and there's basically no perceivable difference with the inverted dropout in this branch. So we're still stuck with inverted dropout being significantly worse. I'm going to try to go back work through the equations and see if I can find an explanation. (Anyone else watching, it'd be great if you could do so as well...) |
|
Sorry to chime in, I have three things to say about the inverted/normal droput thing :
(Accuracy on the test set over 50 epochs for a 3-layers feed forward net with dropout on the first to layers (p=0.2))
|
|
Interesting! Thanks. Maybe the inverted dropout is particularly bad for Gal's form of the dropout, because it is on the same nodes for the entire sequence? Anyhow, I will try to add Gal's version (both inverted and regular) to the Vanilla LSTM, and let's try the benchmarks again.. |
https://arxiv.org/pdf/1512.05287v5.pdf
I'm not 100% sure its correct, and it has some ugliness -- LSTMBuilder now keeps a pointer to ComputationGraph -- but Gal's dropout seems to be the preferred way to do dropout for LSTMs.
Will appreciate another pair of eyes.