Skip to content

Produce the layers using the abstract factory pattern for simplicity#1064

Closed
kloudkl wants to merge 2 commits intoBVLC:devfrom
kloudkl:layer-factory
Closed

Produce the layers using the abstract factory pattern for simplicity#1064
kloudkl wants to merge 2 commits intoBVLC:devfrom
kloudkl:layer-factory

Conversation

@kloudkl
Copy link
Contributor

@kloudkl kloudkl commented Sep 10, 2014

"For me, the lesson learned when writing Caffe as a grad student is simplicity: reduce my problem to one already solved." -- Yangqing

The factory pattern is on the todo list since the public release of Caffe. As more and more implementations are added, it is time to refactor how the layers are created.

The theory is not repeated here as there are already plenty of articles talking about the abstract factory pattern.

@longjon
Copy link
Contributor

longjon commented Sep 10, 2014

This patch seems to trade duplication of the engine selection code for duplication of the layer selection code. I see two drawbacks to this:

  • the layer typing is made looser by allowing every engine to be chosen for every layer, even though most layers have only one; and
  • adding new layers becomes even more cumbersome, and the code duplication grows as the number of engines times the number of layers.

So I'm not sure this is strictly better than what we have now. I would welcome a patch that simply reduced the duplicated engine selection code, and I would also be interested in some way around the current tedium of writing out the protobuf types and C++ types for every layer in layer_factory.cpp...

@shelhamer
Copy link
Member

I think there is some kind of polymorphism possible through protobuf
extensions that might help with the duplication. I haven't had a chance to
look into the details yet.

On Wednesday, September 10, 2014, longjon notifications@github.com wrote:

This patch seems to trade duplication of the engine selection code for
duplication of the layer selection code. I see two drawbacks to this:

  • the layer typing is made looser by allowing every engine to be
    chosen for every layer, even though most layers have only one; and
  • adding new layers becomes even more cumbersome, and the code
    duplication grows as the number of engines times the number of layers.

So I'm not sure this is strictly better than what we have now. I would
welcome a patch that simply reduced the duplicated engine selection code,
and I would also be interested in some way around the current tedium of
writing out the protobuf types and C++ types for every layer in
layer_factory.cpp...


Reply to this email directly or view it on GitHub
#1064 (comment).

Evan Shelhamer

@kloudkl
Copy link
Contributor Author

kloudkl commented Sep 11, 2014

The implementation is changed to be more faithful to the abstract factory pattern to avoid duplication.

@Yangqing
Copy link
Member

Not sure if this really solves the problem here, since the code is essentially the same...

My picture of a registerer will be something that looks like this (this is a random piece of example I found, not saying it is good quality):

https://code.google.com/p/mapreduce-lite/source/browse/trunk/src/base/class_register.h

Such registration allows the users to simply register a class and forget about all others. This comes with drawbacks (have to always link into one single binary, cannot register across binary boundaries, kind-of unstable behavior across platforms), so not sure what is the right direction to go to. Just my 2 cents :)

@Yangqing
Copy link
Member

(This being said, a "real factory" factory is really needed in Caffe :))

@bhack
Copy link
Contributor

bhack commented Sep 12, 2014

I.e. Opencv use the Algorithm class

Copy link
Member

Choose a reason for hiding this comment

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

This parameter is embedded in the respective layer messages for future-proofing: while CAFFE and CUDNN are the only engines at the moment, each layer could have different engines that are not necessarily shared. For instance convolution could pick up an FFT engine but that has no bearing on pooling and the rest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure about the most suitable general way to manage the different implementations of the same layer. The problem is related to #1108.

@Yangqing
Copy link
Member

Just want to provide a pointer to #1167 - with the registry and the registration of creators (something like a factory?) this PR's purpose may partially be fullfilled.

@shelhamer
Copy link
Member

Closing as settled by #1167. Thanks @kloudkl for suggesting an informative alternative.

@shelhamer shelhamer closed this Oct 2, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

Comments