-
Notifications
You must be signed in to change notification settings - Fork 18.6k
pythonic export of features and params for wrapper #112
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
Conversation
note that pairs of params with the same layer name are the params & bias
The code looks fine, although I don't know why we don't use iterators, and in More generally though I'm not sure where the wrapper should land on the spectrum between mirroring exactly the C++ interface and choosing the most Pythonic/convenient interface. We could:
I guess I feel like 3 right now. In any case it seems odd to add the names directly to the blobs, as this changes the semantics of a blob between C++ and Python (plus, as you noted, layer blobs don't have unique names)... |
I'm for the pythonic side of the spectrum. If you're looking for the C++ interface, code in C++. I agree with your 3–I'll figure out how to push an OrderedDict through python::boost. Regarding names in blobs, I did that because it felt awkward to have a list of blob names and a list of blobs and the same for params. Replacing it all with an OrderedDict feels right. Thanks for the feedback. |
I think this is desirable as it is, and refactoring to return an OrderedDict can follow. Let's merge? @longjon |
Sure, I have no problem merging this, as I think it is an improvement, although I hope it's clear to users that the interface is not yet stable... |
With that in mind, I've merged it to |
Fix BVLC#112 entry to proper CUDNN_POOLING_AVERAGE_COUNT_EXCLUDE_PADDING check.
Memory consuming optimization for BN cuDNN algos
Export blobs and params with their names through the python wrapper.
Note that pairs of params with the same layer name are the params & bias.