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

Performance of candleParts #980

Closed
mihakralj opened this issue Dec 21, 2022 · 25 comments
Closed

Performance of candleParts #980

mihakralj opened this issue Dec 21, 2022 · 25 comments
Labels
user support Help a user with their implementation

Comments

@mihakralj
Copy link
Contributor

mihakralj commented Dec 21, 2022

I noticed significant performance difference between calling an indicator and calling a candlePart on the same Quotes object.

For example, on a lengthy list (10,000 bars) the first calc below executes over 100x faster than the second calc:

var SkenderResult = quotes.GetEma(lookbackPeriods: period).Select(i => i.Ema.Null2NaN()!);

var SkenderResult = quotes.GetBaseQuote(CandlePart.HLC3);

We should check what causes such performance issue with GetBaseQuote() method - compared to other GetXXX() methods?

Results from GetBaseQuote() are so slow to calculate and return, that I rather commented them out from my validation tests for HL2, OHLC3 and others. Perhaps I don't call the method the way I should?

Here is the link to my validation tests with your library:
https://github.com/mihakralj/QuanTAlib/blob/dev/Tests/Validations/Trends/Skender.cs

@mihakralj mihakralj added the bug Something isn't working label Dec 21, 2022
@DaveSkender
Copy link
Owner

I have performance metrics for all indicators that use the same base quotes.

Adding this .Select(i => i.Ema.Null2NaN()!); to anything is going to be a lot slower by comparison because you're adding a LINQ query and a transform to each value.

@DaveSkender
Copy link
Owner

DaveSkender commented Dec 21, 2022

Oh, wait, no GetBaseQuote() in that performance data ... I'll add it so it returns performance metrics to monitor better.
Could be something I missed!

@mihakralj
Copy link
Contributor Author

mihakralj commented Dec 21, 2022

let me know; even after .Select() all of your indicators are WAY faster than GetBaseQuote()...

Also, try - just for fun - to switch your library to CPU-native type (Double) instead of Decimal type. Performance increase should be at least 50x. I was shocked when I did comparison between Decimal and Double on my library...

@mihakralj
Copy link
Contributor Author

mihakralj commented Dec 21, 2022

.Select(i => i.Ema.Null2NaN()!);

Do you know a better (direct) way to get a clean (non-nullable) Double list out from your methods?

@DaveSkender
Copy link
Owner

DaveSkender commented Dec 21, 2022

So, here is a quick scan using our usual 500 bars:

EMA: 50 us
BaseQuote: 50 ns

These are fundamentally different complexity, so GetBaseQuote is always going to be a lot faster.
I have some things in the works that'll bring down time in typical indicators for unusually large histories, but they'll never compare to the simplicity of BaseQuote.

@mihakralj
Copy link
Contributor Author

...which means I am doing something unnecessary and wrong when I am calling your GetBaseQuote in my XUnit tests... OK, back to debugging.

@DaveSkender
Copy link
Owner

DaveSkender commented Dec 21, 2022

Do you know a better (direct) way to get a clean (non-nullable) Double list out from your methods?

Doing a for loop is always going to be much faster than any LINQ queries like Select. It's not better syntax, but it is faster:

var results = quotes.GetEma(..).ToList();

for(int i = 0; i < results.Count; i++){
  var r = results[i];
  r.Ema = r.Ema.Null2NaN();
};

@mihakralj
Copy link
Contributor Author

These are test times (comparing your results with mine) - I don't know how to fix atrocious response times for HL2, OHLC3 and others...
image

@DaveSkender
Copy link
Owner

I suppose I could add a utility so you can simply do: results.Null2NaN() to make it easier to convert whole sets.

@DaveSkender
Copy link
Owner

DaveSkender commented Dec 21, 2022

I'm getting 25ms (0.025 sec) for HLC3 on 23,280 bar quotes history (without NaN conversion).

And about 28ms for this EMA on the same large quote history with a NaN conversion:

List<EmaResult> results = big.GetEma(14).ToList();

for (int i = 0; i < results.Count; i++)
{
    EmaResult r = results[i];
    r.Ema = r.Ema.Null2NaN();
}

@mihakralj
Copy link
Contributor Author

mihakralj commented Dec 21, 2022

I added .ToList() at the end of GetCandlePart - and that improved everything dramatically.

@DaveSkender DaveSkender added user support Help a user with their implementation and removed bug Something isn't working labels Dec 21, 2022
@DaveSkender
Copy link
Owner

I added .ToList() at the end of GetCandlePart - and that improved everything dramatically.

Yep, that essentially makes iteration an array operation with for instead of foreach, way faster.

@DaveSkender
Copy link
Owner

DaveSkender commented Dec 25, 2022

If you don't mind working with Collection and tuples, I just noticed we already have a utility to do the NaN conversion on a whole results series.

Collection<(DateTime Date, double? Value)> results = quotes
    .GetEma(..)
    .ToTupleCollection(NullTo.Null2NaN);

@mihakralj
Copy link
Contributor Author

ok, will switch my tests to use this method.

See my progress of libraries cross-validation, all tests are in my test folder: https://mihakralj.github.io/QuanTAlib/#/indicators

@DaveSkender
Copy link
Owner

nice. I see you’ve dinged us on ATR, T3, and Trix. I’d fixed ATR recently for the minor start period issue. Curious what you found on the other two.

@mihakralj
Copy link
Contributor Author

T3: sequential chaining of SMA warm-up values makes no sense (to me) - you initiate each SMA warm-up AFTER previous SMA warm-up and create unnecessary long chain of warming-up (first EMA warms up, THEN second EMA warms up, THEN third...) The first value that you generate is on the bar 6*period-1, and is still very far from the converging value. TA-LIB is taking the same approach.

As T3 is a converging indicator, this approach just wastes bars before convergence. I parallelized the process and am calculating all six EMAs (with leading SMAs) at the same time and am done with SMA approximations within the period. My T3 and your T3 converge within period*10 bars.

I am on the hunt to find Tillson's article in Technical Analysis of Stocks and Commodities magazine from Jan 1998 to see which approach did he take...

@mihakralj
Copy link
Contributor Author

mihakralj commented Dec 26, 2022

TRIX: you are doing just the opposite from T3 - you over-simplified the SMA warm-up:

  • There is a single SMA warm-up for all three EMAs, and there is no attempt to calculate EMA2(EMA1) or EMA3(EMA2) using their own warm-up cycles.
  • having all three EMAs the same at the end of period when an actual calculation starts makes no sense at all.
  • TA-LIB is wasting bars similarly to T3 algo.

I did - just like at T3 - parallelize the warm-up and warm-up all three EMAs independently but within the initial period of bars. Your approach, TA-LIB approach and my approach converge within period*6 bars or so.

I am on the hunt to find Jack Hutson's publication from early '80 to see how did he define TRIX warm-up.

@DaveSkender
Copy link
Owner

Cool, let me know if you find any original author info. I’d thought there was something wrong with these, given the red X, but sounds more you just had some different ideas about the initialization.

@mihakralj
Copy link
Contributor Author

in your implementation, for sure one is wrong - either over-complicated T3 or over-simplified TRIX. But which one? :)

@DaveSkender
Copy link
Owner

Eh, still sounds more like different than wrong. I’ll have to take a closer look at these as well, to better understand what you’re saying. I’m confused since T3 and Trix are different indicators, so they can have different startups and that’s okay, not wrong. If someone were to come up with a newer startup approach, that also wouldn’t make the original wrong.

@mihakralj
Copy link
Contributor Author

They are more similar than you think, and the calc should follow the same logic...

T3 is a clump of 6 EMAs:

Ema1 = Ema (Close);
Ema2 = Ema (Ema1);
Ema3 = Ema (Ema2);
Ema4 = Ema (Ema3);
Ema5 = Ema (Ema4);
Ema6 = Ema (Ema5);
T3 = –(a*a*a) * Ema6 + (3*a*a + 3*a*a*a) * Ema5 + (–6*a*a – 3*a – 3*a*a*a) * Ema4 + (1 + 3*a + a*a*a + 3*a*a) * Ema3

TRIX is a clump of 3 EMAs:

Ema1 = Ema (Close);
Ema2 = Ema (Ema1);
Ema3 = Ema (Ema2);
TRIX = (Ema3-Ema3[1]) / Ema3[1]

@DaveSkender
Copy link
Owner

Have a link to Tillson's T3 S&C article here: https://dotnet.stockindicators.dev/indicators/T3/#content

@mihakralj
Copy link
Contributor Author

Huh. He even wrote a MetaStock code for T3 (on page 3), where he calls Mov() functions in direct sequence one after another, feeding output from one EMA directly to the next.

Knowing how MetaStock works on streaming data, this code will behave exactly like my implementation, warming up all EMAs together for a length of a single period - and not for 6 periods like your implementation.

I feel validated. 😊

@DaveSkender
Copy link
Owner

Yep, that’s what I’m doing too, so we may just be saying the same thing, in violent agreement as the saying goes. I won’t be able to agree or validate anything you’re saying here until I look at it closer.

@mihakralj
Copy link
Contributor Author

mihakralj commented Dec 26, 2022

No, check lines 41, 46, 51, 56, and 61 in https://github.com/DaveSkender/Stock.Indicators/blob/main/src/s-z/T3/T3.Series.cs

In line 61 you are warming up ema6 for SIX periods, instead of warming up all EMAs within a single period.

Looking at Tillson's implementation, all of his T3 EMAs are primed after a single period bars. In your implementation you need a period to prime the first e1, another period to prime e2, another period for e3 and so on till e6. Six times longer, and that becomes a big issue when period is large.

Imagine T3(200) - Tillson's implementation generates first valid result on 201st bar. Yours needs 1200 bars just for a warm-up ...

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
user support Help a user with their implementation
Projects
None yet
Development

No branches or pull requests

2 participants