Stop McClellanOscillator counting one shared bar twice - #9713
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
McClellanOscillator.WarmUpPeriodaddsEMASlow.WarmUpPeriodandADDifference.WarmUpPeriod,and those two overlap by one bar, so it reports 41 while the indicator is ready on 40.
McClellanSummationIndexdelegates to it and reports the same. Subtracting the shared barfixes both.
Related Issue
Closes #9711
Motivation and Context
EMASlowisADDifference.EMA(slowPeriod)andIndicatorExtensions.EMApasseswaitForFirstToReady = true, soEMASlowtakes its first input on the barADDifferencebecomes ready rather than the one after. Adding the periods counts that bar in both.
Nothing the indicator reports moves. At 40 bars the oscillator is 0 and the summation index
is 60, the same as at 41, and
SamplesisWarmUpPeriod * 3either way, so the two valueassertions in those tests hold unchanged.
Requires Documentation Change
No.
How Has This Been Tested?
Both fixtures override
WarmsUpProperlyand assertIsTrue(IsReady)afterWarmUpPeriodbars without asserting it was false before, which is why an early ready passed. Each now
asserts readiness on every bar, the shape
CommonIndicatorTests.WarmsUpProperlyuses.Four inherited tests also read
WarmUpPeriod.TracksPreviousState,WorksWithLowValuesand
IndicatorShouldHaveSymbolAfterUpdatesuse it only as a loop bound of2 * period, andWarmUpIndicatorProducesConsistentResultsfeeds exactlyperiodbars per symbol and thenasserts
IsReady, which this change makes tight rather than slack. Neither fixture overridesany of the four.
My local NUnit host crashes in a Python.NET finalizer before any test runs, because
QuantConnect.pythonnet2.0.65 wants Python 3.11 and this machine has 3.10, the samelimitation I noted on #9694. So this was measured from a console program linked against
QuantConnect.Indicators, feeding three symbols per bar as the fixtures do: ready on bar 40against a reported 41, and reverting the change puts it back. The same program drives and
resets all 190 constructible indicators and finds no other disagreement between
WarmUpPeriodandIsReady.Types of changes
Checklist:
bug-<issue#>-<description>orfeature-<issue#>-<description>