Should we set the phase as TEST when creating the test_net in solver.cpp?

[huangjunshi]

At line 32 of solver.cpp (of master branch), before creating the test_net, I think we should set the phase as TEST (and set back to TRAIN when finish), otherwise it will induce a small bug in the data_layer.cpp if we enable the cropsize parameter at the same time.

Assuming we set the cropsize parameter as positive number. Because the Caffe::TRAIN is the default value of phase_ in class Caffe (line 23, common.cpp), the first batch of testing data will get rand offset (line 53, data_layer.cpp) if we don't set the phase as TEST when creating the test_net.

To double-check, I just add the printing command at line 53 of data_layer.cpp:

... ...
if (Caffe::phase() == Caffe::TRAIN) {
    if (itemid == 0)    LOG(INFO) << "Read training data from: " << layer->db_;
    h_off = rand() % (height - cropsize);
    w_off = rand() % (width - cropsize);
  } else {
... ...

and here is the partial output:

... ...
I0307 19:58:55.497272 36012 solver.cpp:26] Creating training net.
... ...
I0307 19:58:55.752095 36012 net.cpp:70] Creating Layer conv1
I0307 19:58:55.752102 36012 net.cpp:80] conv1 <- data
I0307 19:58:55.752113 36012 net.cpp:105] conv1 -> conv1
I0307 19:58:56.094907 36015 data_layer.cpp:53] Read training data from: 0x1632d60
I0307 19:58:56.094985 36015 data_layer.cpp:53] Read training data from: 0x1632d60
I0307 19:58:56.094985 36015 data_layer.cpp:53] Read training data from: 0x1632d60
I0307 19:58:56.095022 36015 data_layer.cpp:53] Read training data from: 0x1632d60
... ...
I0307 19:58:56.108678 36012 solver.cpp:29] Creating testing net.
... ...
I0307 19:58:56.252768 36012 net.cpp:70] Creating Layer conv1
I0307 19:58:56.252774 36012 net.cpp:80] conv1 <- data
I0307 19:58:56.252782 36012 net.cpp:105] conv1 -> conv1
I0307 19:58:56.252799 36017 data_layer.cpp:53] Read training data from: 0x17452d0
I0307 19:58:56.252854 36017 data_layer.cpp:53] Read training data from: 0x17452d0
... ...

Obviously, the address 0x17452d0 is the address of test_net->db_ (in my case), while the phase is TRAIN.

Pls ignore me if I make any mistake. :-)

[Yangqing]

This may not change too much in terms of accuracy but is indeed a bug. Worth fixing by adding two set_phase calls around the test_net_ initialization code. Thanks for catching this!

[shelhamer]

Perhaps we should do away with a default setting of phase and require it to be set explicitly. This would make intentions clear and avoid subtleties. As well as the issue raised here, currently for deployment (as in the ImageNet example) if one forgets to set the phase and does forward the results will be nondeterministic because of dropout. A simple error of omission that's easily fixed, sure, but one that would be caught by raising an error when the phase is unset.

[huangjunshi]

@shelhamer , also, I think we should avoid using a global variable to indicate the phase when using multi-threading. Maybe the phase should be a member variable of net? Just now, I found another problem if we simply set phase as TEST when creating the test_net, that is, if the data prefetch of train_net is ongoing, and the phase is set as TEST when we start creating the test_net, then the similar problem (train_net reads data with TEST phase) will happen.

[kloudkl]

@huangjunshi, how to reproduce the phase related bug using multi-threading?

[huangjunshi]

@kloudkl , when setup the data layer of trian_net, it actually creates a thread to prefetch the training data. Then, we need to construct the test_net, which also creates a thread in the data layer to prefetch the testing data. In the mean time, if the prefetch of training data has not finished, then there will be a conflict on the phase between the data layers of train_net and test_net.

In my case, because I need to get some additional data as context in the data layer, which costs a few more time. Also, the format of training context and testing context is different. Therefore, the phase related bug may be explicit (and be a big problem) in my case.

[kloudkl]

This bug in fact has the same the root cause with the bugs exposed in #558.

[shelhamer]

I haven't had a chance to do this, but Caffe should complain and die if PHASE isn't set. In my opinion a phase default of TEST could be good to avoid confusion like in #205 too where dropout gives nondeterministic output when simply classifying or feature extracting if you forget set_phase().