#8356 Implemention of NHNL indicators #9109
Conversation
Martin-Molinero
left a comment
Martin-Molinero
left a comment
There was a problem hiding this comment.
Hey @nemcek! Thank you for the PR 💪, leaving some concerns and requests
| private readonly IDictionary<SecurityIdentifier, TradeBar> _currentPeriod = new Dictionary<SecurityIdentifier, TradeBar>(); | ||
| private readonly IDictionary<SecurityIdentifier, Maximum> _rollingPreviousHighs = new Dictionary<SecurityIdentifier, Maximum>(); | ||
| private readonly IDictionary<SecurityIdentifier, Minimum> _rollingPreviousLows = new Dictionary<SecurityIdentifier, Minimum>(); |
There was a problem hiding this comment.
I think instead of having 3 collections per SID, could just have 1 collection with a small private class holdings these 3 attributes, will make the implementation cleaner I think and runtime performance much better
| /// the number of stocks reaching new highs and the number of stocks reaching new lows | ||
| /// in defined time period. | ||
| /// </summary> | ||
| public abstract class NewHighsNewLowsIndicator : TradeBarIndicator, IIndicatorWarmUpPeriodProvider |
There was a problem hiding this comment.
Generally I think 'Difference' and 'Ratio' indicators should not really exist but just be a property of this base indicator, see other indicators like BollingerBands/AverageDirectionalIndex/etc exposing some state through indicatorbase.
Another thought is that ideally this base indicator does not require a TradeBar (which has volume) but requires a Bar, so it would work with QuoteBars, see examples IBaseDataBar/BarIndicator/etc. I would keep a second indicator inheriting from this one which adds volume ratio and enforces tradebar, might need to use some T type handling I think
Minor should remove Indicator from names like NewHighsNewLowsIndicator
| List<TradeBar> newHighStocks = new List<TradeBar>(); | ||
| List<TradeBar> newLowStocks = new List<TradeBar>(); |
There was a problem hiding this comment.
Generally shouldn't create collections unless required, same comment in the foreach above, there's not need to ToList, thinking of performance should be as lazy as possible
| { | ||
| Enqueue(input); | ||
|
|
||
| if (HasMissingCurrentPeriodValue() || !HasSufficientPreviousDataForComputation()) |
There was a problem hiding this comment.
Another thought I think ideally even if not all symbols are ready we could calculate the indicator values with those that are? might even simplify the logic, the indicator state wouldn't be ready yet but have valid value at least
| return new IndicatorResult(vNext); | ||
| } | ||
|
|
||
| private void Enqueue(TradeBar input) |
There was a problem hiding this comment.
Ah I see the indicator is very similar to AdvanceDeclineIndicator generally, seems there is some room to inherit and merge maybe?
2 participants
Description
Implemented the base NHNL indicator as well as Difference, Ratio & Volume Ration NHNL indicators.
The architecture closely follows architecture of ADR indicators.
Related Issue
This PR implements new indicators as in #8356
Motivation and Context
This change adds new indicators.
Requires Documentation Change
Probably adding the new indicators into the list of available indicators.
How Has This Been Tested?
Added unit tests for each new indicator.
Tests closely resemble the tests of ADR indicators.
Also created a test dava (.csv) based on the test data of ADR indicators.
Types of changes
Checklist:
bug-<issue#>-<description>orfeature-<issue#>-<description>