trufanov-nok / ta-lib-rt

TA-Lib RT is a fork of TA-Lib that provides additional API for incremental calculation of indicators without reprocessing whole data.
http://ta-lib.org
83 stars 30 forks source link

talibrt CCI gives different result than talib #14

Closed mdukaczewski closed 3 years ago

mdukaczewski commented 3 years ago
import talibrt
import talib
import numpy as np

timeperiod = 8
data = [1.1] *timeperiod

lows = np.asarray(data)
highs = lows.copy()
closes = lows.copy()

_, state = talibrt.CCI_StateInit(timeperiod=timeperiod)
_, talibrt_ret = talibrt.CCI_BatchState(state, highs, lows, closes)
talib_ret = talib.CCI(highs, lows, closes, timeperiod=timeperiod)
print("talibrt", talibrt_ret[-1], "talib", talib_ret[-1])

returns talibrt 0.0 talib 66.66666666666667

trufanov-nok commented 3 years ago

Hm... When I've heard about CCI i started from googling if I reported a bug about it bcs CCI sounds familiar, but I found nothing.
The problem is that CCI stores average of input data which is (1.1+1.1+1.1)/3=1.1 then calculates average for timeperiod among them, which is exactly the same 1.1. Then calculates sum of stdabs of this average and previous data. Which is supposed to be 0. And after a few other calculations it prints a result:

      if( (tempReal != 0.0) && (tempReal2 != 0.0) )
      {
         outReal[outIdx++] = tempReal/(0.015*(tempReal2/optInTimePeriod));
      }
      else
         outReal[outIdx++] = 0.0;

The problem is that average isn't a zero. It's a 2.220446049250313e-16 due to float numbers implementation in CPU. Which is pretty close to zero but true in if (val != 0.0). You can try in python:

>>> a = 1.1
>>> a
1.1
>>> a = a + a + a + a + a + a + a
>>> a
7.699999999999999
>>> b = a/7
>>> b
1.0999999999999999
>>> 1.1 - b
2.220446049250313e-16

And at the end talib gives you

>>> 2.220446049250313e-16 / (0.015*(1.5543122344752192e-15 / 7))
66.66666666666667

In _State implementation of CCI I have changed the output condition to:

   if( (std_fabs(lastValue) > 1e-13) && (std_fabs(sum) > 1e-13) )
   {
      VALUE_HANDLE_DEREF(outReal) = lastValue/(0.015*(sum/STATE.optInTimePeriod));
   }
   else
       VALUE_HANDLE_DEREF(outReal) = 0.0;

And it's going to the else operations bcs values are smaller than 1e-13.
So, we have two results. 0.0 and 66.66 is quite a different thing.

It looks like I catched this issue, considered it as a bug, fixed in CCI State function but not in the original. Most probably I was going to post a bugreport to the original project and wait for reply. But for some reason I can't find this bugreport now. Perhaps I forgot to post it. They are not replied for years anyway.
So, what do you think about that? Shall I keep the original code untouched?

mdukaczewski commented 3 years ago

Thanks for the quick reply. If you found a bug in the original library then I think it's good you fixed it. However, I think it would be useful to know what changes to the original library logic were made, so there should be at least a note on the project wiki. I know about the changes you made for period = 1 in some functions and the CCI, but there are probably more. And such changes that are not described anywhere can cause consternation. For example, I had a model trained on talib generated data and I wanted to use it with talibrt, so such changes are important to me. I guess I'm not the only one. Talib is faster than talibrt so probably you want to generate datasets for ML with talib, and inference it using talibrt. The ideal solution in my opinion would be to keep the current version as the default, but introduce an additional flag, e.g. #define TALIB_LOGIC_COMPATIBLE and keep both versions in the code.

Finally, I would like to thank you for your work on this library and its publication. If you provide a bitcoin address, I will give you a donation.

trufanov-nok commented 3 years ago

Have just got it! Thanks a lot for your support!