PythonLayer takes parameters by string #2871

Merged
merged 1 commit into from Aug 7, 2015

Conversation

Projects
None yet
5 participants
Owner

shelhamer commented Aug 6, 2015

Cherry-picking of the PythonLayer string parameter from #2001 by @tnarihi to give PythonLayer a param_str member for passing parameters through the python_param proto message.

This is a simple change in itself that leaves the choice of what and how to parameterize open.
There is no automatic eval

shelhamer added the Python label Aug 6, 2015

Owner

shelhamer commented Aug 6, 2015

@philkr have you adapted this test to Python 3 already?

@philkr philkr commented on an outdated diff Aug 6, 2015

python/caffe/test/test_python_layer_with_param_str.py
+ bottom[0].diff[...] = top[0].diff
+ def _forward_mul(self, bottom, top):
+ top[0].data[...] = bottom[0].data * self.value_
+ def _backward_mul(self, bottom, propagate_down, top):
+ bottom[0].diff[...] = top[0].diff * self.value_
+ def reshape(self, bottom, top):
+ top[0].reshape(bottom[0].num, bottom[0].channels, bottom[0].height,
+ bottom[0].width)
+ def forward(self, bottom, top):
+ self._forward(bottom, top)
+ def backward(self, top, propagate_down, bottom):
+ self._backward(bottom, propagate_down, top)
+
+
+def python_net_file():
+ f = tempfile.NamedTemporaryFile(delete=False)
@philkr

philkr Aug 6, 2015

Contributor

NamedTemporaryFile is created with mode='w+b' by default, which in python3 is a buffer, not a string. To fix this just call NamedTemporaryFile(mode='w+', delete=False).

Contributor

philkr commented Aug 6, 2015

The python3 error is easy to fix (see above). I have no idea why python2 fails on those warnings...

Contributor

tnarihi commented Aug 6, 2015

@shelhamer @philkr Thank you so much for taking care of my previous PR! Other than this, I still have a bunch of PRs for python that are not finished. If I need to update and polish them, please let me know.

Contributor

tnarihi commented Aug 6, 2015

According to this post in Stack Overflow, maybe we have to include Python.h first, which means we have to put include of boost/python.hpp first in tools/caffe.cpp. But I am not sure why this warning suddenly came out in this PR.

Contributor

philkr commented Aug 6, 2015

I think ac6d4b6 broke it. It seems to be broken for any PR based on the current master. ac6d4b6 introduced an additional include in caffe.cpp.

Owner

shelhamer commented Aug 6, 2015

Hmm, the branch CI had passed but this is an issue. I'll try to fix it shortly.

Owner

shelhamer commented Aug 6, 2015

@jeffdonahue @longjon does anybody have a favorite name or is param_str_ fine?

Contributor

jeffdonahue commented Aug 6, 2015

I think param_str is fine. It could also just be called param or arg since the str is a little redundant with the protobuf type, but I could also see how it might be a useful hint to have the str annotation when parsing in Python.

shelhamer added the JL label Aug 6, 2015

Owner

shelhamer commented Aug 6, 2015

I changed the name of the property to param_str on the Python side and simplified the test to just check multiplication by the param_str value. @longjon take a look and merge if you like it.

@longjon longjon commented on an outdated diff Aug 7, 2015

python/caffe/test/test_python_layer_with_param_str.py
+ top[0].reshape(*bottom[0].data.shape)
+
+ def forward(self, bottom, top):
+ top[0].data[...] = self.value * bottom[0].data
+
+ def backward(self, top, propagate_down, bottom):
+ bottom[0].diff[...] = self.value * top[0].diff
+
+
+def python_param_net_file():
+ with tempfile.NamedTemporaryFile(mode='w+', delete=False) as f:
+ f.write("""name: 'pythonnet' force_backward: true
+ input: 'data' input_shape { dim: 10 dim: 9 dim: 8 }
+ layer { type: 'Python' name: 'mul10' bottom: 'data' top: 'mul10'
+ python_param { module: 'test_python_layer_with_param_str'
+ layer: 'SimpleParamLayer' param_str: "10" } }
@longjon

longjon Aug 7, 2015

Contributor

Could we be consistent with single-quotes here?

@longjon longjon commented on an outdated diff Aug 7, 2015

src/caffe/proto/caffe.proto
@@ -703,6 +703,12 @@ message PowerParameter {
message PythonParameter {
optional string module = 1;
optional string layer = 2;
+ // This value is set to the attribute `param_str_` of your custom
@longjon

longjon Aug 7, 2015

Contributor

This comment is out-of-date.

Contributor

longjon commented Aug 7, 2015

Looks good except as noted.

Owner

shelhamer commented Aug 7, 2015

Thanks for the glance @longjon. Fixed and merging.

@shelhamer shelhamer added a commit that referenced this pull request Aug 7, 2015

@shelhamer shelhamer Merge pull request #2871 from shelhamer/python-layer-arg
[pycaffe] PythonLayer takes parameters by string
5986a37

@shelhamer shelhamer merged commit 5986a37 into BVLC:master Aug 7, 2015

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

shelhamer deleted the shelhamer:python-layer-arg branch Aug 7, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment