FidoProject / Fido

A lightweight C++ machine learning library for embedded electronics and robotics.
http://fidoproject.github.io/
MIT License
435 stars 81 forks source link

Use builder pattern #39

Open jpetso opened 8 years ago

jpetso commented 8 years ago

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 commented 8 years ago

Thank you for the feedback! @jpetso

truell20 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

@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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

@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 commented 8 years ago

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

joshuagruenstein commented 8 years ago

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.