Allow the python layer have attribute "phase" #3995

Merged
merged 2 commits into from May 4, 2016

Conversation

Projects
None yet
5 participants
Contributor

ZhouYzzz commented Apr 15, 2016

I noticed that the python layer cannot tell whether the phase is TRAIN or TEST. This change may bring convenience in some cases. e.g. to me.

class MyLayer(caffe.Layer):
    ... ...
    def forward(self, bottom, top):
        if (self.phase == caffe.TRAIN):
            # do sth.
        else:
            # do sth. else
    ... ...

shelhamer added the Python label Apr 15, 2016

Contributor

ajtulloch commented Apr 17, 2016

Looks fine to me. Would you be able to add a test that demonstrates reading out the phase with TRAIN/TEST values as well?

Contributor

shaibagon commented Apr 17, 2016

related to issue #3506 -- thanks for implementing this PR!

Contributor

ajtulloch commented Apr 18, 2016

Looks great to me. Can you squash the commits? @longjon , @shelhamer does this look good to you?

@longjon longjon commented on an outdated diff May 4, 2016

python/caffe/test/test_python_layer.py
@@ -44,6 +44,23 @@ def forward(self, bottom, top):
def backward(self, top, propagate_down, bottom):
self.blobs[0].diff[0] = 1
+class PhaseLayer(caffe.Layer):
+ """A layer for checking attribute `phase`"""
+
+ def setup(self, bottom, top):
+ pass
+
+ def reshape(self, bootom, top):
+ top[0].reshape(1)
@longjon

longjon May 4, 2016

Contributor

Scalars should be 0d; use top[0].reshape().

@longjon longjon commented on an outdated diff May 4, 2016

python/caffe/test/test_python_layer.py
@@ -44,6 +44,23 @@ def forward(self, bottom, top):
def backward(self, top, propagate_down, bottom):
self.blobs[0].diff[0] = 1
+class PhaseLayer(caffe.Layer):
+ """A layer for checking attribute `phase`"""
+
+ def setup(self, bottom, top):
+ pass
+
+ def reshape(self, bootom, top):
+ top[0].reshape(1)
+
+ def forward(self, bottom, top):
+ if (self.phase == caffe.TRAIN):
+ top[0].data[...] = 0 # caffe.TRAIN
@longjon

longjon May 4, 2016

Contributor

Please never hardcode the values of symbolic constants (note that these really need to be correct, because they're checked later).

Why not replace this whole forward with top[0].data[()] = self.phase?

@longjon longjon commented on an outdated diff May 4, 2016

python/caffe/test/test_python_layer.py
@@ -140,3 +165,14 @@ def test_parameter(self):
self.assertEqual(layer.blobs[0].data[0], 1)
os.remove(net_file)
+
+ def test_phase(self):
+ net_file = phase_net_file()
+ # Test on phase TRAIN
@longjon

longjon May 4, 2016

Contributor

You can drop these comments; they don't add anything to the code.

(And if you like, make this block a short loop: for phase in caffe.TRAIN, caffe.TEST:...)

Contributor

longjon commented May 4, 2016

I never really got much into phase myself, but this is a clear improvement for the completeness of Python layers, so it seems worthwhile.

However, there are a few style issues, as noted. I'd be happy to merge once these are fixed (and squashed into the existing two commits).

Contributor

ZhouYzzz commented May 4, 2016

@longjon Thanks for review and I've fixed it. I'm glad this pr is helpful.

Contributor

longjon commented May 4, 2016

Looks very clean now, thanks!

@longjon longjon merged commit de8ac32 into BVLC:master May 4, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@fxbit fxbit added a commit to Yodigram/caffe that referenced this pull request Sep 1, 2016

@longjon @fxbit longjon + fxbit Merge pull request #3995 from ZhouYzzz/python-phase
Allow the python layer have attribute "phase"
a16a05c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment