i index overwritten in _arma_predict_out_of_sample

[jbrockmendel]

https://github.com/statsmodels/statsmodels/blob/master/statsmodels/tsa/arima_model.py#L338

    if q:
        i = 0  # if q == 1
    else:
        i = -1

    for i in range(min(q, steps - 1)):

i is set and then promptly ignored. Any idea what it is supposed to do? If it helps, q here is an alias for k_ma.

[josef-pkt]

No idea, maybe an ancient left-over
blame shows the line was moved/changed 5 years ago but that part didn't change.

It might have been used as a shift in the start index for the multistep forecast loop in an older version.

I don't really understand the code, and whenever I need to look into it, it takes me hours to figure out what is or should be going on, especially with this kind of updating loops over parts of arrays.
(i.e. I'm spending enough time on these things to spot a bug candidate during bugfixing, but not enough to think about refactoring)

the part looks like it can be removed

[jbrockmendel]

OK, I'll add this to my list of things to figure out. Tentatively it may be that the loop a few lines below the quoted section may be intended to use i:

    if q:
        i = 0  # if q == 1
    else:
        i = -1

    for i in range(min(q, steps - 1)):
        fcast = (mu[i] + np.dot(arparams, endog[i:i + p]) +
                 np.dot(maparams[:q - i], resid[i:i + q]))
        forecast[i] = fcast
        endog[i+p] = fcast

    for i in range(i + 1, steps - 1):
        fcast = mu[i] + np.dot(arparams, endog[i:i+p])
        forecast[i] = fcast
        endog[i+p] = fcast

Best guess is that the two loops should each have non-i indexes and that i is intended to be used in the range(i+1, steps-1). I'll follow up on this.

[josef-pkt]

check test coverage and an example,
It's often faster to verify that the code "works" even if it is very difficult and time consuming to figure out why it does what it does.
e.g. in this case the indexing depends a lot on whether and where the local endog and resid have already been trimmed. (IIRC from related code then endog and resid start with a past of max(p, q) or max(k_ar, k_ma) observations )

[jseabold]

Just glancing at this and thinking back. It's possible that the first loop never runs. Hence the initialization for the second loop. Sorry, should have left code comments here.

…

On Wed, Jun 28, 2017, 10:52 PM Josef Perktold ***@***.***> wrote: check test coverage and an example, It's often faster to verify that the code "works" even if it is very difficult and time consuming to figure out why it does what it does. e.g. in this case the indexing depends a lot on whether and where the local endog and resid have already been trimmed. (IIRC from related code then endog and resid start with a past of max(p, q) or max(k_ar, k_ma) observations ) — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#3797 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AASE5AySJHAJ2u2KUQ_dbKb2PGCEgKJtks5sIsszgaJpZM4OIhuO> .

[jbrockmendel]

Just trying it out, the first loop definitely does run.

Moreover, changing the indexes in both loops from i to idx (so as to not overwrite i) causes a handful of test failures.

Is the following plausible: the initialization is there specifically in case the first loop doesn't run. The second loop runs over range(i+1, steps-1), so needs i to be defined. If this is correct, then the whole thing could be cleaned up by starting the second loop from i0 = min(k_ma, steps-1)-1.

[jbrockmendel]

Should I bother trying to track this down?

[jseabold]

Hey, sorry my comment wasn't clear. Your plausible is correct.

[jbrockmendel]

@jseabold if I put together a PR for this will you take point on getting it merged? There's a backlog and I don't want to add to jpkt's queue.