Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
y913403515 opened this issue May 30, 2019 · 0 comments
Open

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

y913403515 opened this issue May 30, 2019 · 0 comments

Comments

@y913403515
Copy link

y913403515 commented May 30, 2019

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant