5
\$\begingroup\$

I had an assignment some weeks ago that consisted of making a simple McCulloch-Pitts neural network. I ended up coding it in a pretty OO style (or the OO style I've been taught), and I felt that my implementation might have been a bit excessive for being a one-off assignment: could this perhaps been implemented in a simpler, less verbose way? Possibly forsaking modularity and such since it's not really a concern. I had never seen any Neural Network implementations in Java beforehand, so I didn't really know what was a reasonable approach for this kind of thing.

This was implemented in Java - you were supposed to use C or C++, but I didn't know either of them so the teacher indulged me. It models a neural network with three input neurons, one output neuron and two layers of hidden neurons. The actual input-output specification is probably not important here, so I'll just explain the main class (NeuralNetwork.java); This class holds information about all neurons in the network and mediates all message passing between neurons: the neurons will in each fire one or more of their outgoing neurons in each timestep, based on the sum of the input of the ingoing neurons in the previous timestep. All of this message passing goes through the network: all the neurons don't know about each other or the network.

So the crux of the problem is simply to coordinate neurons and whether or not they fire their synapses in each timestep, and to calculate the responses of the neurons that they fire "towards" (through the synapses).

package main;

import java.util.HashMap;
import java.util.LinkedList;

public class NeuralNetwork {

//for debugging
int timestep = 0;

HashMap<String,InputNeuron> inputNeurons;
LinkedList<Neuron> internalNeurons;
LinkedList<OutputNeuron> outputNeurons;

public NeuralNetwork
    (HashMap<String,InputNeuron> inputNeurons, LinkedList<Neuron> internalNeurons, LinkedList<OutputNeuron> outputNeurons)
{
    this.inputNeurons = inputNeurons;
    this.internalNeurons = internalNeurons;
    this.outputNeurons = outputNeurons;
    this.outputNeurons.get(0).getOutput();
}

public int getNrTimestep() {
    return timestep;
}

public int[] timestep(LinkedList<String> inputs) {
    System.out.println("Network: timestep: " + timestep);
    for (String inp : inputs) {
        assert inputNeurons.get(inp) != null : "timestep was given a String as key for an InputNeuron that didn't exist";
        inputNeurons.get(inp).fire();
    }
    for (Neuron internal : internalNeurons) {
        internal.nextTimestep();
    }
    for (Neuron outputNeuron : outputNeurons) {
        outputNeuron.nextTimestep();
    }
    int[] output = new int[outputNeurons.size()];
    int count = 0;
    for (OutputNeuron outputNeuron : outputNeurons) {
        output[count++] = outputNeuron.getOutput();
    }
    timestep++;
    resetAll();
    return output;
}

private void resetAll() {
    for (Neuron internal : internalNeurons) {
        internal.reset();
    }
    for (Neuron outputNeuron : outputNeurons) {
        outputNeuron.reset();
    }
}

}
\$\endgroup\$
6
\$\begingroup\$

Since you didn't post the code of Neuron, InputNeuron and OutputNeuron so I just give some comments on the style.

Instance variables

private HashMap<String,InputNeuron> inputNeurons;
private LinkedList<Neuron> internalNeurons;
private LinkedList<OutputNeuron> outputNeurons;

Instance variables should be kept private and expose via methods.

Ps: I'm not sure here if you intended to let them package-private for other classess (e.g Neuron, etc) to access them directly.

Prefer reference to objects by their interfaces

It is considered a good practice to reference to objects by their interfaces rather than concrete implementation (if they have one). Here you are using the Collection Framework so it'd be better if you declared the instance variables via interfaces.

private Map<String,InputNeuron> inputNeurons;
private List<Neuron> internalNeurons;
private List<OutputNeuron> outputNeurons;

Make defensive copies

The constructor of NeuronNetwork uses directly the arguments passed to it which could lead to some subtle bugs. It'd be better if you make a copy of those arguments.

public NeuralNetwork(Map<String,InputNeuron> inputNeurons, List<Neuron> internalNeurons,
        List<OutputNeuron> outputNeurons) {
    this.inputNeurons = new HashMap<String, InputNeuron>(inputNeurons);
    this.internalNeurons = new ArrayList<Neuron>(internalNeurons);
    this.outputNeurons = new ArrayList(outputNeurons);
    this.outputNeurons.get(0).getOutput(); // potential IndexOutOfBoundException
}

The last line in the constructor could cause IndexOutOfBoundException if the outputNeurons is empty. I'm not sure the purpose of this line.

Avoid using array as return type if possible

The method timeStep returns an int[] which is an array. IMO this is not good since array is less convenience and doesn't provides much useful behaviors. Returning a Collection or a List would be a better choice.

public Collection<Integer> timestep(List<String> inputs) {
    System.out.println("Network: timestep: " + timestep);

    for (String inp : inputs) {
        assert inputNeurons.get(inp) != null : "timestep was given a String as key for an InputNeuron that didn't exist";
        inputNeurons.get(inp).fire();
    }

    for (Neuron internal : internalNeurons) {
        internal.nextTimestep();
    }

    for (Neuron outputNeuron : outputNeurons) {
        outputNeuron.nextTimestep();
    }

    List<Integer> output = new ArrayList<Integer>(outputNeurons.size());
    for (OutputNeuron outputNeuron : outputNeurons) {
        output.add(outputNeuron.getOutput());
    }

    timestep++;
    resetAll();
    return output;
}

Misc

You could rename the method getNrTimestep() to getTimestep() for more elegant.

That's it. Hope these could help.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the feedback. I didn't really get the benefit of making the defensive copies, though? "Ps: I'm not sure here if you intended to let them package-private for other classess (e.g Neuron, etc) to access them directly." I intended them to be private, but forgot to do it. This class is not used by any of the other classes that model the neural network; it only uses the other classes (neurons). \$\endgroup\$ – Guildenstern Mar 29 '13 at 19:20
  • \$\begingroup\$ Since List is mutable, if you use your arguments directly without making defensive copies, the client code which passes those arguments to NeuronNetwork can modify it directly just as exposing those field with public. You can read more about `Making defensive copies" at Java Journal or Effective Java - Item 39 \$\endgroup\$ – Genzer Mar 30 '13 at 1:48
  • 1
    \$\begingroup\$ Returning Collection is wrong, as the order is relevant. The output is a vector in mathematical sense, so List may do. IMHO an array is best, as you never want it to grow or alike. \$\endgroup\$ – maaartinus Jul 12 '15 at 2:07

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.