Compute correlation (3 types) of stock ticker

Question

I'm developing a web application allowing a logged-in user to find most correlated stocks for selected stock ticker. The user chooses a ticker, period of time (10, 30, 60, 90) and type of correlation (KENDALLS, PEARSONS, SPEARMANS). Then on the server side, the 'magic' happens and the server responds to the client by providing a list of stocks sorted by correlation value.

My controller:

@RequestMapping(value = "/stock/correlation", method = RequestMethod.GET, produces = MediaType.APPLICATION_JSON_VALUE)
@RolesAllowed(AuthoritiesConstants.USER)
public TreeSet<StockStatistic> getCorrelationForSelectedTicker(@RequestParam(value = "correlation_type", required = true) CorrelationType correlationType, @RequestParam(value = "ticker", required = true) StockTicker ticker, @RequestParam(value = "period", required = true) int period) {
    return statisticService.computeCorrelation(ticker, period, correlationType);
}

Here is my service:

1. In the service, I have step and isComputing variables, because when the user starts computing correlation, every second I ask the server if a correlation is computing. I then calculate the percentage of progress and show it to the user via a progress bar. Is my way of informing user about progress correct, or should it be done better?
2. I wanted to use multithreading as I want to save time when computing a correlation. I made an inner class as you can see below. Is it correct?

@Service
public class CorrelationService {

    @Inject
    private StockDetailsRepository stockDetailsRepository;

    private Correlation correlation;
    private int step;
    private boolean isComputing;

    public TreeSet<StockStatistic> computeCorrelation(StockTicker correlationForTicker, int period, CorrelationType correlationType) {
        correlation = getCorrelationImpl(correlationType, correlationForTicker, period, stockDetailsRepository);
        ExecutorService executor = Executors.newFixedThreadPool(5);

        for (StockTicker ticker : correlation.getTickersWithOutOneAnalysed()) {
            executor.submit(new CorrelationStock(ticker));
        }

        executor.shutdown();

        try {
            executor.awaitTermination(1, TimeUnit.HOURS);
        } catch (InterruptedException e) {
            e.printStackTrace();
        }

        this.step = 0;
        this.isComputing = false;
        return correlation.getCorrelationTreeSet();
    }

    private Correlation getCorrelationImpl(CorrelationType correlationType, StockTicker correlationForTicker, int period, StockDetailsRepository stockDetailsRepository2) {
        switch (correlationType) {
        case KENDALLS:
            return new KendallsCorrelationImpl(correlationForTicker, period, stockDetailsRepository);
        case PEARSONS:
            return new PearsonCorrelationImpl(correlationForTicker, period, stockDetailsRepository);
        case SPEARMANS:
            return new SpearmansCorrelationImpl(correlationForTicker, period, stockDetailsRepository);
        }
        return null;
    }

    public int getStep() {
        return step;
    }

    public boolean isComputing() {
        return isComputing;
    }

    class CorrelationStock implements Runnable {
        StockTicker analysedTicker;

        public CorrelationStock(StockTicker analysedTicker) {
            this.analysedTicker = analysedTicker;
        }

        @Override
        public void run() {
            correlation.compute(analysedTicker);
        }
    }
}

Here is my helper class:

public abstract class CorrelationVariables {

    protected StockDetailsRepository stockDetailsRepository;
    protected List<StockTicker> tickersWithOutOneAnalysed;
    protected TreeSet<StockStatistic> correlationTreeSet;
    protected double[] sourceClosePrices;
    protected final int period;

    public CorrelationVariables(StockTicker correlationForTicker, int period, StockDetailsRepository stockDetailsRepository) {
        this.stockDetailsRepository = stockDetailsRepository;
        this.period = period;
        this.correlationTreeSet = new TreeSet<>();
        List<StockDetails> stockDetailsToAnalysed = getContent(correlationForTicker);
        this.sourceClosePrices = getClosePrices(stockDetailsToAnalysed);
        EnumSet<StockTicker> tickersToScan = complementOf(EnumSet.of(correlationForTicker));
        this.tickersWithOutOneAnalysed = new ArrayList<StockTicker>(tickersToScan);
    }

    public List<StockTicker> getTickersWithOutOneAnalysed() {
        return tickersWithOutOneAnalysed;
    }

    public TreeSet<StockStatistic> getCorrelationTreeSet() {
        return correlationTreeSet;
    }

    public void setCorrelationTreeSet(TreeSet<StockStatistic> correlationTreeSet) {
        this.correlationTreeSet = correlationTreeSet;
    }

    public double[] getClosePricesToAnalysed() {
        return sourceClosePrices;
    }

    protected double[] getClosePrices(List<StockDetails> stockDetails) {
        double[] closePrices = new double[stockDetails.size()];

        int index = 0;
        for (StockDetails stds : stockDetails) {
            closePrices[index++] = stds.getClosePrice().doubleValue();
        }

        return closePrices;
    }

    protected List<StockDetails> getContent(StockTicker ticker) {
        return stockDetailsRepository.findByStockTickerOrderByDateDesc(ticker, new PageRequest(1, period)).getContent();
    }
}

And interface:

public interface Correlation {
    void compute(StockTicker ticker);
    List<StockTicker> getTickersWithOutOneAnalysed();
    TreeSet<StockStatistic> getCorrelationTreeSet();
}

And one of my implementation (the second and third implementation looks same but have different variables KendallsCorrelation, kendallsCorrelation, and SpearmansCorrelation spearmansCorrelation):

public class PearsonCorrelationImpl extends CorrelationVariables implements Correlation {

    private PearsonsCorrelation pearsonsCorrelation;

    public PearsonCorrelationImpl(StockTicker correlationForTicker, int period, StockDetailsRepository stockDetailsRepository) {
        super(correlationForTicker, period, stockDetailsRepository);
        pearsonsCorrelation = new PearsonsCorrelation();
    }

    public void compute(StockTicker ticker) {
        double[] closePricesToCompare = getClosePrices(getContent(ticker));
        Double correlation = 0.0;
        if (closePricesToCompare.length == sourceClosePrices.length) {
            correlation = pearsonsCorrelation.correlation(sourceClosePrices, closePricesToCompare);
        }
        StockStatistic stockCorrelation = new StockStatistic(correlation, ticker);
        correlationTreeSet.add(stockCorrelation);
    }
}

1. Is my way of computing the correlation for selected options by user fine, or should it be refactored?
2. I am not sure about creating three separated classes (a lot of duplicated code) for each type of correlation, but I can't figure out another way.

Answer 1

Ok, so there's a lot that can be simplified here.

As Xean points out, your Executor is probably not buying you very much. Don't use it to solve a performance problem until you know you have one, and test thoroughly to validate you're getting a performance boost. If you really need to, make one thread that does all the correlation work. But again, make sure that the computation really is the bottleneck, because I don't think some simple math is going to cause any issues worth the complexity multithreading adds here.

The progress bar support is also probably not necessary. Correlation computations should be fast enough that the bar wouldn't even show up. Don't spend time on this until you know for sure you need it. Make sure you track computation time separately from network latency.

Also, your three classes are really tightly coupled. I took a stab at cleaning things up by breaking the coupling and removing the multithreading and progress bar indicators.

Service class:

@Service
public final class CorrelationService {

    public CorrelationService() {
        super();
    }

    public SortedSet<StockStatistic> computeCorrelation(
            final StockTicker correlationForTicker, 
            final int period, 
            final CorrelationType correlationType) {

        final EnumSet<StockTicker> stocksToCorrelate = EnumSet.allOf(StockTicker.class);
        stocksToCorrelate.remove(correlationForTicker);

        final Correlator correlator = 
                new Correlator(correlationForTicker, period, correlationType.getStrategy());
        return correlator.correlateAll(stocksToCorrelate);
    }
}

Business logic:

public final class Correlator {

    @Inject
    private StockDetailsRepository stockDetailsRepository;

    private final double[] sourceClosePrices;
    private final int period;
    private final CorrelationStrategy correlationStrategy;

    public Correlator(
            final StockTicker ticker,
            final int period,
            final CorrelationStrategy correlationStrategy) {

        this.period = period;
        if (this.period < 1) {
            throw new IllegalArgumentException("Period must be at least 1.");
        }

        this.correlationStrategy = correlationStrategy;
        Objects.requireNonNull(correlationStrategy);

        Objects.requireNonNull(ticker);
        this.sourceClosePrices = this.getClosePrices(ticker);
    }

    public SortedSet<StockStatistic> correlateAll(final Collection<StockTicker> tickers) {
        final SortedSet<StockStatistic> correlations = new TreeSet<StockStatistic>();        
        for (final StockTicker ticker : tickers) {
            correlations.add(this.correlate(ticker));
        }
        return correlations;
    }

    private StockStatistic correlate(final StockTicker ticker) {
        final double[] targetClosePrices = this.getClosePrices(ticker);        
        final Double correlation = 
                correlationStrategy.correlate(sourceClosePrices, targetClosePrices);
        return new StockStatistic(correlation, ticker);
    }

    private double[] getClosePrices(final StockTicker ticker) {
        final List<StockDetails> stockDetailsList = 
                this.stockDetailsRepository.findByStockTickerOrderByDateDesc(ticker, new PageRequest(1, this.period)).getContent();
        final double[] closePrices = new double[stockDetailsList.size()];

        int index = 0;
        for (final StockDetails stockDetails : stockDetailsList) {
            closePrices[index] = stockDetails.getClosePrice().doubleValue();
            index++;
        }

        return closePrices;
    }
}

Strategy definition:

public interface CorrelationStrategy {

    Double correlate(final double[] sourceClosePrices, final double[] targetClosePrices);
}

Abstract Strategy parent. Shared correlation implementation can go here:

public abstract class AbstractCorrelationStrategy implements CorrelationStrategy {

    @Override
    public Double correlate(
            final double[] sourceClosePrices,
            final double[] targetClosePrices) {

        if (sourceClosePrices.length != targetClosePrices.length) {
            return 0.0;
        }
        return doCorrelate(sourceClosePrices, targetClosePrices);
    }

    protected abstract Double doCorrelate(
            final double[] sourceClosePrices,
            final double[] targetClosePrices);

}

Answer 2

For me it is not easy to give you any hints related to stock related stuff but maybe some general thoughts to think about:

About multithreaded web service applications:

• how frequently are you receiving requests?
• How many different resources (files, network, cpu time) will be used by the computation of the Correlations?
• How much time will such a computation need? You give them up to one hour. This is very long for a request.

And this is why: if you are going to get many requests per second (even minute) and all the computation (which maybe take some seconds) only needs cpu time but no I/O, then you do not get any performance boost (depending on your hardware ), but it'll be slower. Let's say you have 7 request at the same time. For each request you allow 5 threads for calculation, so you have 35 active threads. Your hardware can handle 8 or maybe 16 threads in parallel, then the threads wait for cpu time. Context switching, many cache misses all the time. You have to test if this improves your performance.

The getClosePrices() returns a array of doubles. The ordering of this array depends on the ordering of the stockDetails list. Maybe this is ok in this context but in general this is not very clean (IMO). Each close price should be dependent on its StockDetails like with a HashMap.

The PearsonCorelationImpl inherit CorrelationVariables and Correlation. Dose this make sense? It seems like other implementations will inherit the same, so you probably could combine the parent class and the interface.

You have a getter for the sourceClosePrices, so you should use this instead of the protected variable. In general I wouldn't use protected members inherited from the parent class (use getters instead). I searched for the definition of those variables in the class and it needed a second before I looked into the parent class member definitions. It confused me.

In PearsonCorelationImpl.compute() you have a variable named correlation of the class type Double. Why isn't this a double?

You have the interface Correlation. I think it is confusing to have a variable named correlation which is a Double and not a Correlation and a method correlation() which isn't returning a Correlation. Maybe you should rename the interface (i.e. CorrelationCalculator)

Hope you have some thoughts to think about now ;-)