6
\$\begingroup\$

I wrote a class that I'm using to calculate conditional probabilities of a given distribution as well as perform naive Bayes classification. I'd like to get a code review done to tell me if there is anything that I can do to make what I wrote neater or perform better.

Probability class:

package edu.uba.filters;

import java.util.*;

import org.apache.commons.math3.util.Pair;


public class Probability {

protected Frequency<String> iFrequency = new Frequency<String>();
protected Frequency<String> rFrequency = new Frequency<String>();
private String[] targetClassKeys;
private HashMap<String,Double> priors = new HashMap<String, Double>();
private LinkedList<Pair<String,Double>> predictions = new     LinkedList<Pair<String,Double>>();


public Probability(){
    super();
}

public void setInterestedFrequency(List<String> interestedFrequency){
    for(String s: interestedFrequency){
        this.iFrequency.addValue(s);
    }
}

public void setReducingFrequency(List<String> reducingFrequency){
    for(String s:reducingFrequency){
        this.rFrequency.addValue(s);
    }
}

public LinkedList<Pair<String,Double>> getPredictions(){
    return this.predictions;
}

/*
 *return conditional probability of P(interestedClass|reducingClass)
 */
public double conditionalProbability(List<String> interestedSet,
                                     List<String> reducingSet,
                                     String interestedClass,
                                     String reducingClass){
    List<Integer> conditionalData = new LinkedList<Integer>();
    double returnProb = 0;
    iFrequency.clear();
    rFrequency.clear();

    this.setInterestedFrequency(interestedSet);
    this.setReducingFrequency(reducingSet);


    for(int i = 0;i<reducingSet.size();i++){
        if(reducingSet.get(i).equalsIgnoreCase(reducingClass)){
            if(interestedSet.get(i).equalsIgnoreCase(interestedClass)){
                conditionalData.add(i);
            }
        }
    }

    int numerator = conditionalData.size();
    int denominator = this.rFrequency.getNum(reducingClass);

    if(denominator !=0){
        returnProb = (double)numerator/denominator;
    }

    iFrequency.clear();
    rFrequency.clear();
    return returnProb;
}

public void naiveBayes(Data data,List<String> targetClass, BayesOption bayesOption){
    //intialize variables
    int numOfClasses = data.getHeaders().size();
    Object[] keyNames = data.getHeaders().toArray();
    double conditionalProb = 1.0;
    double prob = 1.0;
    String[] rClass;
    String priorName;


    iFrequency.clear();
    rFrequency.clear();

    if(bayesOption.compareTo(BayesOption.TRAIN) == 0){
        this.setInterestedFrequency(targetClass);
        this.targetClassKeys = Util.convertToStringArray(iFrequency.getKeys());

        for(int i=0;i<this.targetClassKeys.length;i++){
            priors.put(this.targetClassKeys[i], iFrequency.getPct(this.targetClassKeys[i]));
        }
    }


    //for each classification in the target class
    for(int i=0;i<this.targetClassKeys.length;i++){

        //get all of the different classes for that variable
        for(int j=0;j<numOfClasses;j++){

            String reducingKey = Util.convertToString(keyNames[j]);
            List<String> reducingClass = data.getData().get(reducingKey);
            this.setReducingFrequency(reducingClass);
            Object[] reducingClassKeys = rFrequency.getKeys();
            rClass = Util.convertToStringArray(reducingClassKeys);


            for(int k=0;k<reducingClassKeys.length;k++){

                if(bayesOption.compareTo(BayesOption.TRAIN) == 0){
                    conditionalProb = conditionalProbability(targetClass, reducingClass, this.targetClassKeys[i], rClass[k]);
                    priorName = this.targetClassKeys[i]+"|"+rClass[k];
                    priors.put(priorName,conditionalProb);
                }

                if(bayesOption.compareTo(BayesOption.PREDICT) == 0){
                    priorName = this.targetClassKeys[i]+"|"+rClass[k];
                    prob = prob * priors.get(priorName);

                }
            }
            rFrequency.clear();

        }

        if(bayesOption.compareTo(BayesOption.PREDICT) == 0){
            prob = prob * priors.get(this.targetClassKeys[i]);
            Pair<String,Double> pred = new Pair<String, Double>(this.targetClassKeys[i],prob);
            this.predictions.add(pred);
        }

    }

    this.iFrequency.clear();
    this.rFrequency.clear();

}

public String getNaiveBayesPrediction(){
    Collections.sort(predictions, new Comparator<Pair<String, Double>>() {
        public int compare(Pair<String, Double> o1, Pair<String, Double> o2) {
            return o2.getValue().compareTo(o1.getValue());
        }
    });
    return this.predictions.get(0).getKey();
}

}

My probability class relies really heavily on a Frequency class:

package edu.uba.filters;

import com.google.common.collect.LinkedListMultimap;
import com.google.common.collect.Multimap;
import com.google.common.collect.Multiset;
import com.google.common.collect.HashMultiset;

import java.util.Collections;
import java.util.Comparator;
import java.util.Iterator;
import java.util.Set;

public class Frequency<T extends Comparable<T>> {

private Multiset event = HashMultiset.create();
private Multimap event2 = LinkedListMultimap.create();

public void addValue(T data){
    if(event2.containsKey(data) == false){
        event2.put(data,data);
    }

    event.add(data);
}

public void clear(){

    this.event = null;
    this.event2 = null;

    this.event = HashMultiset.create();
    this.event2 = LinkedListMultimap.create();
}

public double getPct(T data){
    int numberOfIndElements = event.count(data);
    int totalNumOfElements = event.size();
    return (double) numberOfIndElements/totalNumOfElements;
}

public int getNum(T data){
    int numberOfIndElements = event.count(data);
    return numberOfIndElements;
}

public int getSumFreq(){
    return event.size();
}

public int getUniqueCount(){
    return event.entrySet().size();
}

public String[] getKeys(){
    Set<String> test = event2.keySet();
    Object[] keys = test.toArray();
    String[] keysAsStrings = new String[keys.length];

    for(int i=0;i<keys.length;i++){
        keysAsStrings[i] = (String) keys[i];
    }

    return keysAsStrings;
}
}
\$\endgroup\$
  • \$\begingroup\$ Hi! Welcome to Code Review. Good job on your first post! \$\endgroup\$ – TheCoffeeCup Nov 10 '15 at 0:10
4
\$\begingroup\$
public class Frequency<T extends Comparable<T>> {

private Multiset event = HashMultiset.create();
private Multimap event2 = LinkedListMultimap.create();

The first problem that I'd note is that the indent is off. Consider adding a level of indent inside the class like this

public class Frequency<T extends Comparable<T>> {

    private Multiset<T> event = HashMultiset.create();
    private Multimap<T> event2 = LinkedListMultimap.create();

Sometimes this is a copy/paste problem. If you select an entire block of code that contains at least one line with no indent, using Ctrl+K will add indent to the entire block. If all the lines already have indent, it decreases the indent.

Another issue in this section of code is that Multiset and Multimap should be parameterized.

Probability

import java.util.*;

It's generally better to import each class separately. This imports everything in java.util whether you use it or not. Many modern IDEs will handle imports for you, so it may not even be more work.

protected Frequency<String> iFrequency = new Frequency<String>();
protected Frequency<String> rFrequency = new Frequency<String>();

If you are using an up-to-date Java, you don't need to write out the parameters:

protected Frequency<String> iFrequency = new Frequency<>();
protected Frequency<String> rFrequency = new Frequency<>();

It will figure out the matching type for you.

private HashMap<String,Double> priors = new HashMap<String, Double>();
private LinkedList<Pair<String,Double>> predictions = new     LinkedList<Pair<String,Double>>();

It's more common to define variables as interfaces rather than implementations, so

private Map<String,Double> priors = new HashMap<>();
private List<Pair<String,Double>> predictions = new LinkedList<>();

It's possible that you are dealing with one of the exceptions to this with predictions, but I don't see it in this code. With priors, you aren't using any HashMap specific functionality and you don't allow access to it directly.

The point of this is to make it easier to change the implementations later. Will it ever matter in this code? Maybe not. But you will eventually write some code where changing the implementation makes sense. If you develop the habit now, that will be easier later.

    public Probability(){
        super();
    }

You don't have to do this. Java automatically calls the no argument super for you. Also, Java will create a default no argument constructor for you. You only have to make a constructor if you want a different behavior from the default.

    public LinkedList<Pair<String,Double>> getPredictions(){
        return this.predictions;
    }

Again, you don't need to write out LinkedList. List is sufficient.

    public List<Pair<String,Double>> getPredictions(){
        return predictions;
    }

You also don't need to say this. unless you are resolving a conflict. I didn't notice any usages of this. that were actually needed.

    iFrequency.clear();
    rFrequency.clear();

    this.setInterestedFrequency(interestedSet);
    this.setReducingFrequency(reducingSet);

This is a weird pattern. You normally set object fields once, in the constructor. They contain data that is a characteristic of the object and allow you to access that data in multiple methods without passing it around as parameters. But you're not doing that here. You declare these as object fields but then you clear and initialize them inside a method. Then you clear them again at the end. Why not just declare them as local variables?

    Frequency<String> iFrequency = new Frequency<>();
    Frequency<String> rFrequency = new Frequency<>();

Then they will automatically disappear at the end of the method. This also makes concurrency easier, but that may not matter here. As a general rule, you should always define variables at the smallest scope required. That can save accidental conflicts later.

You can then change your two setFrequency methods to one addAll method on Frequency. So you can say

    iFrequency.addAll(interestedSet);
    rFrequency.addAll(reducingSet);

No need to maintain two identical methods.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the comments!!! I've gone back and started to make some edits to my code. As to your last point the reason that I'm constantly setting and clearing the frequency variables is because with some functions I need to be able to store the frequencies. You can see that more clearly in the naiveBayes function. I use the same function to train the classifier as well as to make predictions, so I need to be able to access the probabilities after training so I just made those two things attributes of the class. I'm open to other suggestions though. \$\endgroup\$ – j.jerrod.taylor Nov 10 '15 at 13:26

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.