Skip to content

Conversation

@philkr
Copy link
Contributor

@philkr philkr commented Aug 5, 2015

This PR allows the python interface to access the top and bottom blob names, which makes the data flow a bit more explicit in python.

@longjon longjon added the Python label Aug 5, 2015
@philkr philkr force-pushed the top_bottom_names branch 3 times, most recently from 22b07de to 1e7cc54 Compare August 9, 2015 16:55
@philkr philkr force-pushed the top_bottom_names branch 2 times, most recently from b32022a to 7d4890a Compare August 17, 2015 20:50
@longjon
Copy link
Contributor

longjon commented Aug 24, 2015

Seems like a reasonable thing to expose to pycaffe. A couple questions:

  1. Why expose these functions in C++ as well? Currently they're (obviously) not in use; if there's some refactoring to be done using them they might be worth having, but if not it seems we're exposing an unused interface.
  2. Why create custom classes instead of using OrderedDicts like elsewhere? Right now this seems like an unnecessary (and less functional) mixing of styles.

@philkr
Copy link
Contributor Author

philkr commented Aug 24, 2015

thanks for the feedback. As for the C++ exposition, I didn't see a way of exposing it to python without exposing something to C++, but I'm open to suggestions.
(2) I guess an ordered dict of lists could work, but I felt it might not be as easy as what is there. The main issue is that currently the top and bottom names are computed in the function Net<Dtype>::bottom_names. In order to put them into a OrderedDict one would need to call this function for every layer and store the result somewhere in the python Net, which can get messy quickly. Please let me know if you have any suggestions on how to handle this without creating a mess.

@philkr philkr force-pushed the top_bottom_names branch 2 times, most recently from bbe3bd1 to a723331 Compare August 28, 2015 23:01
@philkr philkr force-pushed the top_bottom_names branch 2 times, most recently from bbf2565 to f578605 Compare September 18, 2015 21:16
@philkr philkr force-pushed the top_bottom_names branch 2 times, most recently from b811915 to a711733 Compare October 30, 2015 20:16
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also need to CHECK_GE(i, 0), yes?

@longjon
Copy link
Contributor

longjon commented Jan 5, 2016

The new patch looks pretty reasonable; comments as noted. Also:

  • I'm still not sure that constructing an OrderedDict, as used by many of the other Net methods would be messier than the current approach; the code would be pretty much as it is now (perhaps in comprehension form), cached in net if needed per your comment above. Then you could (e.g.) iterate through top_names in order, which is a fairly straightforward thing to desire, but won't work right now.
  • Please squash cleanups before merge.

@philkr
Copy link
Contributor Author

philkr commented Jan 5, 2016

Not sure where to compute the OrderedDict to store it in net. I have been using this for a while now. I usually iterate through _layer_names and layers anyway and rarely directly through top and bottom names. But if you see an easy change to make it work I'm very open to changing it.

@longjon
Copy link
Contributor

longjon commented Jan 5, 2016

Right, due to the awkwardness of the current method rewriting scheme, it would need to be done in a similar way to the other Net methods, i.e., you would compute the OrderedDict during the call. If that creates unacceptable per-call overhead, you could cache the result in the net object (e.g., by testing hasattr). Which maybe we should be doing on other calls as well, or maybe we should provide an __init__ callback, or use some other scheme, but this is the way it is now...

longjon added a commit that referenced this pull request Jan 8, 2016
Exposing layer top and bottom names to python
@longjon longjon merged commit 917ef33 into BVLC:master Jan 8, 2016
@longjon
Copy link
Contributor

longjon commented Jan 8, 2016

Okay, I'll go ahead and merge as is; while it would be nice to have OrderedDict functionality, this PR provides a useful interface and that functionality can be added in a future patch.

Thanks for this exposure @philkr!

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants