Skip to content
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

Use builder pattern #39

Open
jpetso opened this issue Apr 27, 2016 · 10 comments
Open

Use builder pattern #39

jpetso opened this issue Apr 27, 2016 · 10 comments

Comments

@jpetso
Copy link

jpetso commented Apr 27, 2016

Trying to read your example code without comments or the documentation handy in a browser window is a pretty puzzling experience. Lots of numeric values without a hint as to what that value might be. Many developers who do not comment their code all that well will leave an unreadable mess when using Fido.

For instance, give this to someone who might have heard about the basics of machine learning but can't remember all the details (let alone the order of parameters in the Fido API):

net::NeuralNet neuralNetwork = net::NeuralNet(1, 1, 2, 4, "sigmoid");
net::Backpropagation backprop = net::Backpropagation(0.1, 0.001, 0.001, 10000);

Nobody knows what this means without looking up the docs. Now imagine if the code looked like this:

net::NeuralNet neuralNetwork = net::NeuralNet::Builder()
    .numInputs(1).numOutputs(1)
    .numHiddenLayers(2).numNeuronsPerHiddenLayer(4)
    .activationFunction("sigmoid")
    .build();

net::Backpropagation backprop = net::Backpropagation::Builder()
    .learningRate(.0.1)
    .momentumTerm(0.001)
    .acceptableErrorLevel(0.001)
    .maxTrainingIterations(10000))
    .build();

Not only does the coder get autocompletion and can initialize the object without double-checking the docs, but the reader now also has a clue what's going on. It's not even much longer than the current version with comment, but now everyone will write readable code and not just those that take the time to write a helpful comment.

This is called the builder pattern, Wikipedia has an article on it too. I think it would make a great match for many of the classes in your API.

@truell20
Copy link
Member

truell20 commented Apr 27, 2016

Thank you for the feedback! @jpetso

@truell20
Copy link
Member

truell20 commented Apr 27, 2016

I do agree that the initializers do need to be more self documenting, but is the builder design pattern common in C++? I have mostly seen it used with Java.

@jpetso
Copy link
Author

jpetso commented Apr 28, 2016

Qt uses it in a number of places. The GoF Design Patterns book and Wikipedia both include if, and provide a C++ implementation. It was definitely meant for C++ as well, although the Java community took the whole design patterns thing to a bit closer to heart.

It might not be the most common one, but it's fairly easy to grasp and not overly hard to implement. It's a solution to a well-defined problem, and as far as I'm aware there's not much competition to that in C++ land.

The most common alternative, I guess, would be to make everything a setter of an already-initialized object, rather than building a valid object before initialization. This has other trade-offs (i.e. necessity to check object validity before performing an action) and is best suited for a slightly different use case.

I would also say that because it's easy enough to come up with almost-good-enough alternatives, the builder pattern hasn't seen as much popularity as it deserves. In my opinion, that's not a reason to avoid using it :)

@truell20
Copy link
Member

@jpetso. Yes, but if we went with the "making everything a setter design scheme," I believe that all current models would be valid with preset constants, which removes the need to check object validity.

@jpetso
Copy link
Author

jpetso commented Apr 29, 2016

If that's not a problem then regular setters might work just fine. The main thing that a builder does for you is force the user to supply all the necessary values so they can be passed to the (private) constructor at once.

@hmwildermuth
Copy link
Collaborator

I like the builder pattern because, especially for me, being new to machine learning, it makes it easier to understand the class and its members.

@joshuagruenstein
Copy link
Member

joshuagruenstein commented Apr 29, 2016

While initially I found the builder pattern (or a similiar design scheme) verbose and java-y, it has grown on me and I recognize its practicality. I especially would agree with @FlyingGraysons that making the switch would make the library more accessible to beginners. Thank you @jpetso for the suggestion.

@truell20
Copy link
Member

@joshuagruenstein @FlyingGraysons. A getter and setter design scheme would look like this:

net::NeuralNet neuralNetwork;
neuralNetwork.setNumInputs(1);
neuralNetwork.setNumOutputs(1);
neuralNetwork.setNumHiddenLayers(2);
neuralNetwork.setNumNeuronsPerHiddenLayer(4);
neuralNetwork.setActivationFunction("sigmoid");

@truell20
Copy link
Member

Or we could have each setter return the object, allowing for one liners.

@joshuagruenstein
Copy link
Member

I actually think the builder pattern is clearer and less verbose than a setter system, but that's just me. I also dislike how you could create a neural network without specifying the number of inputs and outputs, that just seems wrong to me.

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

No branches or pull requests

4 participants