-
Notifications
You must be signed in to change notification settings - Fork 18.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve python wrapper #311
Conversation
Let's also make sure everything that should has a docstring once the interface is stable. |
@longjon re: gpu memory, it seems it's not actually an issue. Although I don't know what problem I had before. This seems fine, unless I'm tired and somehow missing the issue. |
Yes, I can confirm that, e.g., this works in GPU mode: net.params['fc8'][0].data[...] = 0
net.ForwardPrefilled() # or your preferred forward call and this fits my model of what p = net.params['fc8'][0].data
net.ForwardPrefilled()
p[...] = 0
net.ForwardPrefilled() # uses unchanged parameters which makes me a bit uncomfortable. However, getting blobs has the same issue. In fact, in this code bl = net.blobs['fc8'].data
net.ForwardPrefilled()
# bl does not contain up-to-date fc8, and writing to it has no effect but in this code like a reference: bl = net.blobs['fc8'].data
net.ForwardPrefilled()
bl2 = net.blobs['fc8'].data
# bl now contains the same data as bl2, and writing to it works I think it's fine and probably the best compromise to keep things like they are, but to maximize sanity one should consider |
ilsvrc_2012_mean.npy has dims K x H x W. Code written for the old D x D x K mean needs to be rewritten!
Do forward pass by prefilled or packaging input + output blobs and returning a {output blob name: output list} dict.
Preserve the non-batch dimensions of blob arrays, even for singletons. The forward() and backward() helpers take lists of ndarrays instead of a single ndarray per blob, and lists of ndarrays are likewise returned. Note that for output the blob array could actually be returned as a single ndarray instead of a list.
@@ -1,6 +1,6 @@ | |||
// Copyright 2014 BVLC and contributors. | |||
// pycaffe provides a wrapper of the caffe::Net class as well as some | |||
// caffe::Caffe functions so that one could easily call it from Python. | |||
// caffe::Caffe functions so that one could easily call it from python. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really? See python.org, Python wiki page, etc. Just sayin'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, fair enough. Capital is fine and used throughout now.
Always the standard for Python is four-space indents (PEP8 or Google style guide). |
# Input preprocessing | ||
Net.mean = {} # image mean (ndarray, input dimensional or broadcastable) | ||
Net.input_scale = {} # for a model that expects data = input * input_scale | ||
Net.channel_swap = {} # for RGB -> BGR and the like |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the axis permutation also be part of this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
H x W x K is the scikit-image standard, so I'm comfortable leaving it out. It'd be easy to add on the model of the existing options if it turns out to be annoying.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, H x W x K is pretty much the standard for images, I am thinking of non-image data. But I agree we don't need to be eager to add things that aren't used.
Some time ago I suggested (which I am now recalling and still partial to) moving the blob copying in forward from C++ to Python. This would mean that for k, v in kwargs.iteritems():
self.blobs[k].data[...] = np.asarray(v)
net._forward_prefilled()
# now deal with output where The advantages of doing it this way:
What do you think? |
Final note for now, regarding the
The fourth (and optionally the second?) saves time if the user wants some intermediate level features and not later ones. Of course, that option can be implemented in addition to one of the others. And three related comments:
|
This is equally reasonable, and one can ask for outputs + additional blobs by
While true, it feels a bit awkward, and if we're going to have a convenience method let's make it convenient.
This is a good idea... for the future. For now the usual workaround of defining the decapitated net and running it is fine (ok, more like "barely tolerable").
I'm not sure how to fix it right now, so let's document it. We should make docs pages for the python wrapper and matlab wrapper in general with details like this.
Right now you're yelled at if you don't pass all the named input blobs.
I can relate–the collusion of named keywords + **kwargs feels like sin. But it's nice and I've made my peace. |
This is an excellent idea. I'll do this, barring unforeseen issues. |
Take blob args and give blob returns as single ndarrays instead of lists of arrays. Assign the net blobs and diffs as needed on the python side, which reduces copies and simplifies the C++ side of the wrapper. Thanks @longjon for the suggestion.
...and refer to inputs as inputs and not images since general vectors and matrices are perfectly fine.
Don't run scripts in the module dir to avoid import collisions between io and caffe.io.
For compositionality and expectations.
Improve python wrapper
This work-in-progress seeks to polish the python wrapper, document, and include more detailed examples.
fix handling of gpu memoryseems like gpu blob access is alright for now (provided Forward or Backward are called, since they will trigger syncing) see Improve python wrapper #311 (comment)