signaflo / java-timeseries

Time series analysis in Java
MIT License
195 stars 49 forks source link

Attention: there might be some mistake in the code. #36

Open y913403515 opened 5 years ago

y913403515 commented 5 years ago

Hi, thanks for your java-timeseries working! I'm reading this open source recently. I found that there might be some mistake in the code. Please see the class ArimaCoefficients.

There may be errors in the calculation process of the methods expandArCoefficients(...) and
expandMaCoefficients(...).

For example,

double[] arCoeffs = new double[] {1, 3, 5};
double[] sarCoeffs = new double[] {2};
int seasonalFrequency = 2;

double[] expandArcoeffs = new double[arCoeffs.length + sarCoeffs.length * seasonalFrequency]; 
expandArcoeffs = expandArCoefficients(arCoeffs, sarCoeffs, seasonalFrequency);

By the method expandArCoefficients(...), the array expandArcoeffs is euqal to
{1.0, 2.0, -2.0, -6.0, -10.0}. In fact, through simple polynomial operations, the array expandArcoeffs should be equal to {1.0, 5.0, 3.0, -6.0, -10.0}.

Furthermore, if arCoeffs.length >= seasonalFrequency, then the method expandArCoefficients(...) may result in wrong results. And if maCoeffs.length >= seasonalFrequency, then the method expandMaCoefficients(...) may result in wrong results.

In my opinion, the correct code may be as follows.

static double[] expandArCoefficients(final double[] arCoeffs, final double[] sarCoeffs,
                                         final int seasonalFrequency) {
        double[] arC = new double[arCoeffs.length+1];
        double[] sarC = new double[sarCoeffs.length+1];
        double[] arSarCoeffs = new double[arCoeffs.length + sarCoeffs.length * seasonalFrequency];
        double[] arSarC = new double[arSarCoeffs.length+1];

        arC[0] = -1.0;
        sarC[0] = -1.0;
        System.arraycopy(arCoeffs, 0, arC, 1, arCoeffs.length);
        System.arraycopy(sarCoeffs, 0, sarC, 1, sarCoeffs.length);

        // Note that we take into account the interaction between the seasonal and non-seasonal coefficients,
        // which arises because the model's ar and seasonal ar polynomials are multiplied together.
        for (int i = 0; i < arC.length; i++) {
            for (int j = 0; j < sarC.length; j++) {
                arSarC[i + j* seasonalFrequency] += -arC[i] * sarC[j];
            }
        }
        System.arraycopy(arSarC, 1, arSarCoeffs, 0, arSarCoeffs.length);
        return arSarCoeffs;
    }

    // Expand the moving average coefficients by combining the non-seasonal and seasonal coefficients into a single
    // array, which takes advantage of the fact that a seasonal MA model is a special case of a non-seasonal
    // MA model with zero coefficients at the non-seasonal indices.
    static double[] expandMaCoefficients(final double[] maCoeffs, final double[] smaCoeffs,
                                         final int seasonalFrequency) {

        double[] maC = new double[maCoeffs.length+1];
        double[] smaC = new double[smaCoeffs.length+1];
        double[] maSmaCoeffs = new double[maCoeffs.length + smaCoeffs.length * seasonalFrequency];
        double[] maSmaC = new double[maSmaCoeffs.length+1];

        maC[0] = 1.0;
        smaC[0] = 1.0;
        System.arraycopy(maCoeffs, 0, maC, 1, maCoeffs.length);
        System.arraycopy(smaCoeffs, 0, smaC, 1, smaCoeffs.length);

        // Note that we take into account the interaction between the seasonal and non-seasonal coefficients,
        // which arises because the model's ar and seasonal ar polynomials are multiplied together.
        for (int i = 0; i < maC.length; i++) {
            for (int j = 0; j < smaC.length; j++) {
                maSmaC[i + j * seasonalFrequency] += maC[i] * smaC[j];
            }
        }
        System.arraycopy(maSmaC, 1, maSmaCoeffs, 0, maSmaCoeffs.length);
        return maSmaCoeffs;
    }

I hope I misunderstood your code.

Please forgive my poor English and code. Thank you again for your open source. It's excellent.

Best wishes!