Reject an ARIMA period too short for its own orders - #9758
Open
mkzung wants to merge 1 commit into
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
AutoRegressiveIntegratedMovingAveragechecksperiod < Math.Max(arOrder, maOrder), which is weaker than whatComputeNextValuereads:arrayData[arOrder - 1]and_residuals[2 * maOrder], over a seriesDifferenceSeriesshortens. The guard now asks forMath.Max(arOrder, 2 * maOrder + 1) + Math.Max(diffOrder, 0), and its message follows the wordingBeta,CorrelationandValueAtRiskalready use.The clamp on
diffOrderis there because nothing validates it as non-negative andDifferenceSeriesruns only when it is positive, so a negative one costs no samples.Related Issue
Closes #9757.
Motivation and Context
168 of the 534 order sets the old guard admits throw on the bar that fills the window, 127 at line 209 and 41 at line 201, before the indicator reports ready.
HandleExceptionsdoes not cover them: it wraps the two fitting steps, and both reads are inComputeNextValue. With the change 366 of the 600 are admitted and none throws, so the same 168 are refused and nothing that works today is.Requires Documentation Change
No.
How Has This Been Tested?
RejectsAPeriodTooShortForItsOrderstakes six order sets, two of them with a negativediffOrder, and asserts both sides of the boundary and the exception text. The ARIMA fixture is 22 passed; reverting the guard fails four cases and reverting the clamp fails the other two.IndicatorResetContractTestsgives 1140 passed and 72 skipped on this branch and on master alike, which is where a stricter constructor would show since it builds every indicator at periods 1, 2 and 14. Every ARIMA in the repository is built at period 50, where nothing changes either way.