bukosabino / ta

Technical Analysis Library using Pandas and Numpy
https://technical-analysis-library-in-python.readthedocs.io/en/latest/
MIT License
4.25k stars 867 forks source link

DonchianChannel problem #133

Open yomoko opened 4 years ago

yomoko commented 4 years ago

Hi, I think there's something wrong with the donchian channel method.

  1. It's one period off. Eg, If you use a broker chart (with the same D-Channel) to compare with the python calculated result, it's off. I have to use shift(1) to make the date sync with the correct result.
  2. Even if we specify n=100 and fillna=False, there's no na value (the first n values) in the result
  3. For hband, we actually have to use ta.volatility.DonchianChannel(high) and lband, ta.volatility.DonchianChannel(low), which is two different operation. But in the documentation and they way how it's structured, it says to use 'close' column, which is very mis-leading and wrong.

Why is that? Thanks.

bukosabino commented 4 years ago

Hi @yomoko ,

Thank you for your advice. I will do some tests to check the results of the Donchian Channel asap.

Best, Dario

yomoko commented 4 years ago

@bukosabino For pt 1, the definition should be the past n-period high/low. NOT current n-period. If it uses current n-period, the indicator will never return 1 (Because the price can never close above and below the channel).

matteosantama commented 4 years ago

I'm not sure I agree with your interpretation of the indicators for pt 1). The indicator you're suggesting is simply whether we see an N day high or low

bukosabino commented 4 years ago

Hi @yomoko ,

I have worked on the DonchianChannel indicator. I have added some useful functions and fix some minor details. By answering your comments:

  1. I have checked the Donchian Channel implementation, and I think it is correct. I don't agree with your interpretation of the indicator. I think we need to include the last n-periods (including the current close value). Can you share with me what broker chart implements the indicator using your way? Of course, I am open to discuss this topic.

I have developed some test to check the results, and they match with this Google Sheet doc: https://docs.google.com/spreadsheets/d/17JWWsxSiAb24BLzncUpccc8hg-03QjVWVXmoRCJ2lME/edit#gid=0

Please, check it and let me know if there is something wrong.

  1. I have fixed the fillna statements. Now, if you set the fillna=False you will get n NaN values.

  2. You don't need the low or high series. The way to use the indicator would be:

indicator = ta.volatility.DonchianChannel(close=df['close'], n=20, fillna=False)
df['d_channel_high'] = indicator.donchian_channel_hband()
df['d_channel_low'] = indicator.donchian_channel_lband()
df['d_channel_middle'] = indicator.donchian_channel_mband()
df['d_channel_width'] = indicator.donchian_channel_wband()
df['d_channel_percentage'] = indicator.donchian_channel_pband()
df['d_channel_high_indicator'] = indicator.donchian_channel_hband_indicator()
df['d_channel_low_indicator'] = indicator.donchian_channel_lband_indicator()

To use all of this, you need to update the last version (0.5.21) of the library:

pip install -U ta

or

pip install ta==0.5.21

Best, Dario

yomoko commented 4 years ago

@bukosabino
Well... since you have point 3 wrong in the first place (lowest low and highest high of past n-period, not highest close and lowest close of past n-period), you don't see any problem with point 1.

bukosabino commented 4 years ago

Hi @yomoko ,

You are right, I should calc the highest high and the lowest low. Let me try a new proposal.

Best, Dario

yomoko commented 4 years ago

@bukosabino Great. Then you will see that if you do so as point 1, nothing will actually close ABOVE or BELOW the channel, which means the following will only return 0: image

Because the max you get is close = hband(current close = current high) and the min close = lband(current close = current low) if current period is being used. But the definition is actually LAST n-period. A lot of charting programs out that are actually wrong. Eg. tradingview & metatrader

bukosabino commented 4 years ago

I have fixed the indicator:

In your case, if you want the last n-periods, you should call the indicator:

from ta.volatility.DonchianChannel

indicator = ta.volatility.DonchianChannel(high=df['high'], low=df['low'], close=df['close'], n=20, offset=1, fillna=False)
df['d_channel_high'] = indicator.donchian_channel_hband()
df['d_channel_low'] = indicator.donchian_channel_lband()
df['d_channel_middle'] = indicator.donchian_channel_mband()
df['d_channel_width'] = indicator.donchian_channel_wband()
df['d_channel_percentage'] = indicator.donchian_channel_pband()

The results of the tests matches with these documents:

  1. Using offset = 0 https://docs.google.com/spreadsheets/d/17JWWsxSiAb24BLzncUpccc8hg-03QjVWVXmoRCJ2lME/edit?usp=sharing

  2. Using offset = 1 https://docs.google.com/spreadsheets/d/1BnYRoP9cjy-SP9TKnkTwJtUhUuZeDQbzXMQHNsNPSC4/edit?usp=sharing

It is available in the last library version (0.5.22).

pip install -U ta

or

pip install ta==0.5.22

Thank you for your help. Please, let me know if you see anything wrong!

Best, Dario

yomoko commented 4 years ago

@bukosabino It looks good. But still.. I can't think of a reason why offset=0 is correct. But since there's an option to change it, I guess no point to argue if it should be 1-period delayed or not.

bukosabino commented 4 years ago

Hi @yomoko ,

Could you provide me some link/information about the offset should be 0 or 1?

Best, Dario

yomoko commented 4 years ago

@bukosabino https://www.incrediblecharts.com/indicators/donchian_channels.php image As I said, a lot of big guys have it wrong for whatever reason...

There's nothing wrong with your deleted channel band indicator, but since you have your previous point 1 wrong, that's what made the indicator doesn't make sense. I have said that numerous times already.

And if you still don't get it, as I said in the previous reply, forget it.