search

askmike / gekko

A bitcoin trading bot written in node - https://gekko.wizb.it/
MIT License
10.07k stars 3.94k forks source link

Combining built-in indicators with TA-lib indicators #837

Closed imkane closed 7 years ago

imkane commented 7 years ago

I realize this is probably a dumb question, but I'm not sure why, if I copy-paste the /strategies/RSI.js code into a new custom strategy .js file, then simply add:

this.addTalibIndicator('myadx', 'adx', { optInTimePeriod: settings.ADX.period });

Within the init() method, the backtesting results change drastically. Don't I have to actually use the ADX indicator results within the check() method for the backtest to change? Why, simply by adding the indicator do the results change?

askmike commented 7 years ago

Don't I have to actually use the ADX indicator results within the check() method

Yes in theory there should not be any difference, could you please share specific backtests?

imkane commented 7 years ago

Sure thing!

poloniex | USDT | XRP 2017-01-01 00:00 to 2017-03-01 00:00

[RSI] interval = 6 low = 36 high = 69 persistence = 1

Adding that single line in init() changes the simulated profit from 100% to -81%

imkane commented 7 years ago

Is anyone else able to confirm this behavior?

askmike commented 7 years ago

I will test asap!

I can tell you that as soon as you add a talib indicator the execution flow changes slightly (since these indicators are now calculated in async). But if that results in different outputs it is a bug.

imkane commented 7 years ago

Mike, I know you're super busy and already put in a ridiculous amount of work for this project (thank you!). I was sorta hoping someone else could give it a try, as it should only take a few mins (assuming the import is done) :)

Where's everyone else at? lol

thegamecat commented 7 years ago

Ok, I have not experienced this.

Can you upload your two strats and config files somewhere and I will test it.

Edit; interesting I never put the indicator add line anywhere but init. I think if you put it in check you are effectively not getting the indicators time component. So an ema20 will always be an ema1 in check. I could be wrong but that's what I guess.

imkane commented 7 years ago

Here's my files: https://github.com/imkane/gekko

FYI this was backtested via the UI.

In config.js:

config.Kane2 = {
    RSI: {
        interval: 8,
        low: 29,
        high: 68,
        persistence: 1
    },
    ADX: {
        period: 10,
        value: 25
    }
};

In Kane2.js, un-comment line 37 to see the results change: this.addTalibIndicator('myadx', 'adx', { optInTimePeriod: settings.ADX.period });

Thanks for your help!

imkane commented 7 years ago

@thegamecat any luck?

thegamecat commented 7 years ago

Nope sorry. Will try tomorrow!

imkane commented 7 years ago

No worries! I haven't been using Gekko since I ran into this, because if something so basic doesn't work as expected, I have no confidence in the rest of the app lol. Hopefully I'm just doing something wrong.

thegamecat commented 7 years ago

I've run your code with ADX disabled and enabled.

ADX commented out: 2017-07-19 11:06:12 (INFO): (PROFIT REPORT) start time: 2017-01-07 19:25:00 2017-07-19 11:06:12 (INFO): (PROFIT REPORT) end time: 2017-07-06 06:25:00 2017-07-19 11:06:12 (INFO): (PROFIT REPORT) timespan: 6 months 2017-07-19 11:06:12 (INFO): 2017-07-19 11:06:12 (INFO): (PROFIT REPORT) start price: 9.91056006 USDT 2017-07-19 11:06:12 (INFO): (PROFIT REPORT) end price: 263 USDT 2017-07-19 11:06:12 (INFO): (PROFIT REPORT) Market: 2553.73498983% 2017-07-19 11:06:12 (INFO): 2017-07-19 11:06:12 (INFO): (PROFIT REPORT) amount of trades: 6337 2017-07-19 11:06:12 (INFO): (PROFIT REPORT) original simulated balance: 1.00000000 USDT 2017-07-19 11:06:12 (INFO): (PROFIT REPORT) current simulated balance: 0.00000000 USDT 2017-07-19 11:06:12 (INFO): (PROFIT REPORT) simulated profit: -1.00000000 USDT (-100.00000000%) 2017-07-19 11:06:12 (INFO): (PROFIT REPORT) simulated yearly profit: -2.03389831 USDT (-203.38983051%)

ADX enabled:

2017-07-19 11:07:56 (INFO): (PROFIT REPORT) start time: 2017-01-07 19:25:00 2017-07-19 11:07:56 (INFO): (PROFIT REPORT) end time: 2017-07-06 06:25:00 2017-07-19 11:07:56 (INFO): (PROFIT REPORT) timespan: 6 months 2017-07-19 11:07:56 (INFO): 2017-07-19 11:07:56 (INFO): (PROFIT REPORT) start price: 9.91056006 USDT 2017-07-19 11:07:56 (INFO): (PROFIT REPORT) end price: 263 USDT 2017-07-19 11:07:56 (INFO): (PROFIT REPORT) Market: 2553.73498983% 2017-07-19 11:07:56 (INFO): 2017-07-19 11:07:56 (INFO): (PROFIT REPORT) amount of trades: 703 2017-07-19 11:07:56 (INFO): (PROFIT REPORT) original simulated balance: 1.00000000 USDT 2017-07-19 11:07:56 (INFO): (PROFIT REPORT) current simulated balance: 0.85309498 USDT 2017-07-19 11:07:56 (INFO): (PROFIT REPORT) simulated profit: -0.14690502 USDT (-14.69050200%) 2017-07-19 11:07:56 (INFO): (PROFIT REPORT) simulated yearly profit: -0.29878987 USDT (-29.87898712%)

imkane commented 7 years ago

Well shit :(

@thegamecat can you please try using this:

poloniex | USDT | XRP 2017-01-01 00:00 to 2017-03-01 00:00

imkane commented 7 years ago

Oh wait, I misread what you posted!

Yours is also showing different numbers simply by including the ADX indicator but not using it. Thanks @thegamecat for confirming.

@askmike I think we found a bug ...

askmike commented 7 years ago

I have just tested with an extra talib.adx and I see the same results, this is very strange. I'll stop what I was working on with gekko and fix this asap first.

imkane commented 7 years ago

Thanks Mike! I wish I was smart enough to help debug!

thegamecat commented 7 years ago

Talib does expand the dataset for sure. I've written indicators that don't use talib but need the richer dataset.

askmike commented 7 years ago

Okay I've figured out the problem:

Gekko is written in javascript. Whenever you do any kind of IO (any type of "call" to something outside your javascript code) you do that call and specify what you want to happen once it is done (at a later stage). If you use talib indicators in your strat gekko will do a call to talib to calculate the indicators, once this is done Gekko will give control to your strat (including the indicator results). However by the time TAlib gave Gekko the results of an indicator for a given candle, the non talib indicators might be from the "next candle".

This bug is a race condition which only affects strats that have:

imkane commented 7 years ago

Interesting ... good catch!

askmike commented 7 years ago

I just commited a hotfix, can you verify whether it works for you?

imkane commented 7 years ago

Much better!

One thing that's still changing is the Sharpe Ratio:

No ADX: no adx ADX: adx

askmike commented 7 years ago

Could you double check if the roundtrips (table below the chart) are the same? Because sharpe ratio is calculated based on these.

imkane commented 7 years ago

This is strange, when I run it repeatedly with ADX the Sharpe Ratio changes: -0.86, -0.7, -0.75, etc

The Rountrips table is different too. The rightmost 4 columns have different values.

ADX: adx No ADX: no adx

askmike commented 7 years ago

This is most likely a similar bug that manifested itself in a very different part of the codebase, a part that has changed a lot (esp for #737). I'll look into this asap.

thegamecat commented 7 years ago

This is odd. If I pull your updated basetradingmethod file into my gekko I get a segmentation fault on the db when running a backtest through cli.

Have reverted back to previous version now.

askmike commented 7 years ago

That's really strange, the baseTradingMethod does not interact with any database..

On Fri, Jul 21, 2017 at 12:08 PM, thegamecat notifications@github.com wrote:

This is odd. If I pull your updated basetradingmethod file into my gekko I get a segmentation fault on the db when running a backtest through cli.

Have reverted back to previous version now.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/askmike/gekko/issues/837#issuecomment-316974071, or mute the thread https://github.com/notifications/unsubscribe-auth/AA7MDzPq-TcO77eu-OTRKbeyzs_iMtnXks5sQIafgaJpZM4OSJp8 .

-- PGP key at keybase.io/mikevanrossum https://keybase.io/mikevanrossum/key.asc

thegamecat commented 7 years ago

I know, I am utterly confused. I've tried different dbs and trading methods and get the same result.

I need to run the full latest code base - I've been waiting for your major update before doing it though :)

askmike commented 7 years ago

I need to run the full latest code base - I've been waiting for your major update before doing it though :)

Ah yes, sorry about the delays. Extremely busy at the moment, loads of stuff for work (different clients), a lot going on personally. In these cases I unfortunately have to defer working on Gekko..

thegamecat commented 7 years ago

HAHA I didn't mean it that way - just that I am waiting :)

imkane commented 7 years ago

@thegamecat I'm using 'feature/ui-trader' branch with only difference being the updated baseTradingMethod.js file. No segmentation fault noticed. Running CentOS.

thegamecat commented 7 years ago

Ta, I'll try that when I can. It's worrying because I've got custom strats doing really well with your current master branch and no idea how this will pan out!

thegamecat commented 7 years ago

Quick update on this - I've installed and tested the feature/ui-trader branch and it's working fine with my strat which includes standard and talib indicators. I ran master and feature/ui side by side and got slightly improved trades with the feature/ui.

Edit: re-running this, I think it's down to a different slippage value. Will see.

What I've concluded is that change to the baseTradingMethod clearly causes a problem with the master codebase for me.

Looking forward to the merge with develop.

thegamecat commented 7 years ago

Another edit....I've just realised the feature/ui branch doesn't have the base trading fix in it....

I've added the base trading fix into both Master and Feature/ui and theyr're both running now. We will see how long for - it's a lot slower is my initial finding.

thegamecat commented 7 years ago

Feature/ui branch just failed:

<--- Last few GCs --->

[3865:0x26d5a50] 677113 ms: Mark-sweep 1400.0 (1439.4) -> 1400.0 (1439.4) MB, 882.2 / 0.1 ms (+ 1.1 ms in 1 steps since start of marking, biggest step 1.1 ms) allocation failure GC in old space requested [3865:0x26d5a50] 678569 ms: Mark-sweep 1400.0 (1439.4) -> 1400.0 (1438.4) MB, 1455.4 / 0.3 ms last resort gc [3865:0x26d5a50] 680025 ms: Mark-sweep 1400.0 (1438.4) -> 1400.0 (1438.4) MB, 1455.4 / 0.2 ms last resort gc

<--- JS stacktrace --->

==== JS stack trace =========================================

Security context: 0x29f6fe6c0d39 2: inspect [util.js:187] [pc=0x2a413782a5bc](this=0x2b68d612d7b9 <an Object with map 0x34993fc09639>,obj=0x1ef197897869 <String[114]\: \n SELECT from candles_USD_BTC\n WHERE start <= 1456533239 AND start >= 1456530240\n ORDER BY start ASC\n >,opts=0x29f6fe604421 ) 3: arguments adaptor frame: 3->2 4: / anonymous */ [/mnt/f/bash/newgekko/gekko/node_modules/sqlite3/...

FATAL ERROR: CALL_AND_RETRY_LAST Allocation failed - JavaScript heap out of memory 1: node::Abort() [node] 2: 0x126426c [node] 3: v8::Utils::ReportOOMFailure(char const, bool) [node] 4: v8::internal::V8::FatalProcessOutOfMemory(char const, bool) [node] 5: v8::internal::Factory::NewByteArray(int, v8::internal::PretenureFlag) [node] 6: v8::internal::SourcePositionTableBuilder::ToSourcePositionTable(v8::internal::Isolate, v8::internal::Handle) [node] 7: v8::internal::FullCodeGenerator::MakeCode(v8::internal::CompilationInfo) [node] 8: 0xd00385 [node] 9: 0xd0157c [node] 10: 0xd07f3f [node] 11: v8::internal::Compiler::Compile(v8::internal::Handle, v8::internal::Compiler::ClearExceptionFlag) [node] 12: v8::internal::Runtime_CompileLazy(int, v8::internal::Object*, v8::internal::Isolate) [node] 13: 0x2a41373063a7 Aborted (core dumped)

thegamecat commented 7 years ago

And now Master branch fails:

<--- Last few GCs --->

[3871:0x2e19a40] 784283 ms: Mark-sweep 1399.9 (1437.2) -> 1399.8 (1437.2) MB, 1657.6 / 0.0 ms allocation failure GC in old space requested [3871:0x2e19a40] 785953 ms: Mark-sweep 1399.8 (1437.2) -> 1399.8 (1436.2) MB, 1669.1 / 0.0 ms last resort gc [3871:0x2e19a40] 787579 ms: Mark-sweep 1399.8 (1436.2) -> 1399.8 (1436.2) MB, 1626.2 / 0.0 ms last resort gc

<--- JS stacktrace --->

==== JS stack trace =========================================

Security context: 0x3feff7bc0d39 2: exec [native regexp.js:~113] [pc=0x10cf4dd5d871](this=0x10cfad88fd41 ,C=0x14f91659fee1 <Number: 1.46881e+12>) 3: makeDateFromInput [/mnt/f/bash/gekko/node_modules/moment/moment.js:1348] [pc=0x10cf4dd9ac6b](this=0x14f9165af039 ,config=0x37f39f004d89 <an Object with map 0x15c440e11c49>) 4: makeMoment [/mnt/f/bash/gekko/node_modules/moment/moment.js:1515...

FATAL ERROR: CALL_AND_RETRY_LAST Allocation failed - JavaScript heap out of memory 1: node::Abort() [node] 2: 0x126426c [node] 3: v8::Utils::ReportOOMFailure(char const, bool) [node] 4: v8::internal::V8::FatalProcessOutOfMemory(char const, bool) [node] 5: v8::internal::Factory::NewRawOneByteString(int, v8::internal::PretenureFlag) [node] 6: v8::internal::Factory::NewStringFromOneByte(v8::internal::Vector, v8::internal::PretenureFlag) [node] 7: v8::internal::Factory::NumberToString(v8::internal::Handle, bool) [node] 8: v8::internal::Runtime_NumberToStringSkipCache(int, v8::internal::Object*, v8::internal::Isolate) [node] 9: 0x10cf4d908506 Aborted (core dumped)

askmike commented 7 years ago

JavaScript heap out of memory

You don't have enough memory, this is not an issue with gekko.

thegamecat commented 7 years ago

Memory in what sense?

This is a 32GB machine.

I'm running this on Develop now, will see what happens, but it's definitely trading differently which is expected.

thegamecat commented 7 years ago

Same thing on Develop branch: <--- Last few GCs --->

[5372:0x2c15a50] 794084 ms: Mark-sweep 1399.3 (1436.4) -> 1399.3 (1436.4) MB, 1712.7 / 0.0 ms allocation failure GC in old space requested [5372:0x2c15a50] 795781 ms: Mark-sweep 1399.3 (1436.4) -> 1399.3 (1435.4) MB, 1697.0 / 0.0 ms last resort gc [5372:0x2c15a50] 797496 ms: Mark-sweep 1399.3 (1435.4) -> 1399.3 (1435.4) MB, 1715.5 / 0.0 ms last resort gc

<--- JS stacktrace --->

==== JS stack trace =========================================

Security context: 0x4063c1c0d39 1: _extend [util.js:~973] [pc=0x17904561076d](this=0x2bc09432d711 <an Object with map 0x366b1ec09639>,target=0x25279227a961 <an Object with map 0x398ff0307011>,source=0x25279227a931 <an Object with map 0x366b1ec88b49>) 2: getOptions(aka getOptions) [fs.js:47] [pc=0x179045622997](this=0x4063c104311 ,options=0x4063c1c9949 <String[4]: utf8>,defaultOptions=0x25279227a931 <an Objec...

FATAL ERROR: CALL_AND_RETRY_LAST Allocation failed - JavaScript heap out of memory 1: node::Abort() [node] 2: 0x126426c [node] 3: v8::Utils::ReportOOMFailure(char const, bool) [node] 4: v8::internal::V8::FatalProcessOutOfMemory(char const, bool) [node] 5: v8::internal::Factory::NewTransitionArray(int) [node] 6: v8::internal::TransitionArray::Insert(v8::internal::Handle, v8::internal::Handle, v8::internal::Handle, v8::internal::SimpleTransitionFlag) [node] 7: v8::internal::Map::CopyReplaceDescriptors(v8::internal::Handle, v8::internal::Handle, v8::internal::Handle, v8::internal::TransitionFlag, v8::internal::MaybeHandle, char const, v8::internal::SimpleTransitionFlag) [node] 8: v8::internal::Map::CopyAddDescriptor(v8::internal::Handle, v8::internal::Descriptor, v8::internal::TransitionFlag) [node] 9: v8::internal::Map::CopyWithField(v8::internal::Handle, v8::internal::Handle, v8::internal::Handle, v8::internal::PropertyAttributes, v8::internal::Representation, v8::internal::TransitionFlag) [node] 10: v8::internal::Map::TransitionToDataProperty(v8::internal::Handle, v8::internal::Handle, v8::internal::Handle, v8::internal::PropertyAttributes, v8::internal::Object::StoreFromKeyed) [node] 11: v8::internal::LookupIterator::PrepareTransitionToDataProperty(v8::internal::Handle, v8::internal::Handle, v8::internal::PropertyAttributes, v8::internal::Object::StoreFromKeyed) [node] 12: v8::internal::StoreIC::LookupForWrite(v8::internal::LookupIterator, v8::internal::Handle, v8::internal::Object::StoreFromKeyed) [node] 13: v8::internal::StoreIC::UpdateCaches(v8::internal::LookupIterator, v8::internal::Handle, v8::internal::Object::StoreFromKeyed) [node] 14: v8::internal::StoreIC::Store(v8::internal::Handle, v8::internal::Handle, v8::internal::Handle, v8::internal::Object::StoreFromKeyed) [node] 15: v8::internal::KeyedStoreIC::Store(v8::internal::Handle, v8::internal::Handle, v8::internal::Handle) [node] 16: v8::internal::Runtime_KeyedStoreIC_Miss(int, v8::internal::Object*, v8::internal::Isolate) [node] 17: 0x1790451063a7 Aborted (core dumped)

thegamecat commented 7 years ago

So, the question is - why does this happen with the basetrading change?

It's not happening with the previous version.

I don't think this is a closed issue as the fix is causing a problem.

askmike commented 7 years ago

@thegamecat this is definitely a closed issue since:

Combining built-in indicators with TA-lib indicators Within the init() method, the backtesting results change drastically. Don't I have to actually use the ADX indicator results within the check() method for the backtest to change? Why, simply by adding the indicator do the results change?

is now fixed. If you think the fix introduced a new issue, please open a new issue and provide more information (out of heap is definitely memory related, so please post what you are trying to do as well as Gekko output (as opposed to v8 stack traces).

thegamecat commented 7 years ago

Ok, will do.

talentoscope commented 6 years ago

Looks like this has reappeared in some form, issue #1689