anandanand84 / technicalindicators

A javascript technical indicators written in typescript with pattern recognition right in the browser
MIT License
2.18k stars 563 forks source link

MFI calculation wrong? #127

Closed ghost closed 6 years ago

ghost commented 6 years ago

I was testing MFI on BTCUSD daily, and noticed that investing.com, tradingview, and cryptowat.ch all return similar values, but they are different from what this library is generating.

I digged in the code, and I have a feeling there's a mistake in calculating the Typical price. Based on this:

Typical Price = (High + Low + Close)/3

But the library seems to NOT divide by 3. I tried to divide this number by 3 manually, but this did not seem to affect the results. Any idea what might be causing the disprecancy in what this library returns, and what the other websites return?

Thx!

anandanand84 commented 6 years ago

can you give me a screenshot of tradingview comparing against https://cryptotrading-hub.com (uses this library). As for your typeical price, it does divide by 3. see the link below.

https://github.com/anandanand84/technicalindicators/blob/master/src/chart_types/TypicalPrice.ts#L23

JMoli commented 6 years ago

Hi Andy, make sure you enabled proper precision if you are going to use this library for crypto trading

https://github.com/anandanand84/technicalindicators#precision

anandanand84 commented 6 years ago

The test case for MFI is based on the stock charts link you provided https://github.com/anandanand84/technicalindicators/blob/master/test/volume/MFI.js and the test pass so either the calculation provided in the stockcharts is wrong or it should be wrong in the other websites. If you think stock charts is wrong then we may need the actual logic to fix.

ghost commented 6 years ago

1525317642

1525317576

There we go. As you can see, I hovered over the same candles, and the MFI values are not the same. You can also notice that the MFI value before we reach 20k hits 100 on tradingview, but to 87 on this library.

JMoli commented 6 years ago

Are the inputs the same? I can see a value of 14 on one but not the other

anandanand84 commented 6 years ago

@andyperisho Thanks for taking the time to get the screenshot. Agreed there is a bug and I looked the code and figured out where the bug is. One last thing can you get me values of high, low, close and the mfi for me to write the test case. Currently this library compares the typical price against close which should be against previous typical price, can you get me the test case so I could make the fix.

ghost commented 6 years ago

@JMoli yes they are the same. It's BTC/USDT for binance, so i doubt they would differ :)

@anandanand84 I see u closed the issue. So is this resolved, or do u still need the information u requested?

anandanand84 commented 6 years ago

It is resolved, but you check again and confirm.

ghost commented 6 years ago

Could u publish it to npm?

anandanand84 commented 6 years ago

it is published

ghost commented 6 years ago
npm ERR! code ETARGET
npm ERR! notarget No matching version found for technicalindicators@^1.1.11
ghost commented 6 years ago

And on npm, looks like it's still at 1.1.10: https://www.npmjs.com/package/technicalindicators

anandanand84 commented 6 years ago

Sorry my bad I thought I published it. Published now.

ghost commented 6 years ago

Confirmed and tested. The results match what i'm seeing on trading view. Thx a lot mate.

I'm curious, did the change impact the tests u had from the traditional market data?

anandanand84 commented 6 years ago

The problem was the test from the stockcharts.com had computation from typical price and not from high, low, close. So I manually created high, low, close to to the same value to get the same typical price. So in my test case typical price and close had the same value so it passed when I compared it with the prev close instead of prev typical price since because both are same.

https://github.com/anandanand84/technicalindicators/blob/master/test/volume/MFI.js#L11

ghost commented 6 years ago

That makes sense.

In any case, thank you very much for your swift response!